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

Updated dependencies #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

onionhammer
Copy link
Contributor

I updated the dependencies in the platform ini file. The biggest change here is just hardcoding the unix timestamp to max int (2038). I think this is safe, since in order to use the library you have to give the device its device key anyways.

Long term, I think it would be better long term to instead of calculating the SAS token on the device, this should be given to the device during its provisioning process, and re-acquired after it expires; in otherwords put the onus on the user of the library to supply a valid SAS token, rather than supply a device key. This is ultimately more secure, since you aren't required to actually give a device its access key.

@andrei-markeev
Copy link

But in this case you would need to reupload SAS token to the devices every time it is expired? There can be thousands of devices located thousands kilometers apart, not easy to access. I think this introduces more problems than it solves...

@onionhammer
Copy link
Contributor Author

There is no expiration

@andrei-markeev
Copy link

Do you mean that it is possible to set token expiration to far future?
I would still prefer generating tokens anew from time to time to be honest, it is better from security point of view...

One thing I found btw is that NTP support is now moved to the core, so NTPClient library becomes obsolete:
esp8266/Arduino#4122

@andrei-markeev
Copy link

Here's the example code using this new functionality (this by the way also eliminates Arduino "Time" library dependency):

void setup()
{
    // ...
    settimeofday_cb(onTimeRetrieved);
    configTime(0, 0, "pool.ntp.org");
    WiFi.begin(wifiName, wifiPassword);
    // ...
}

void onTimeRetrieved (void)
{
  Serial.printf("Current timestamp: %d\n", time(NULL));
  client.onEvent(onClientEvent);
  // then you can call client.begin, etc.
}

Having this, we can safely remove all code related to NTP from AzureIoTHubMQTTClient, and finally use time(NULL) instead of now() - so that include <TimeLib.h> can also be removed.

@onionhammer
Copy link
Contributor Author

It shouldn't be up to the device to create an SAS token, the token should be provided by your provisioning service, and the implementor to reconnect

@andrei-markeev
Copy link

andrei-markeev commented Aug 4, 2018

Microsoft seems to think differently, all official examples that I saw, generate SaS tokens on device. Security is the primary concern when it comes to corporate world...

Well anyway, this is my opinion, you have yours, I respect it. Just wanted to point out that the approach you took is less secure, and that fetching time became much easier in latest versions of esp8266 core.

@onionhammer
Copy link
Contributor Author

onionhammer commented Aug 5, 2018

Unfortunately the approach taken in this library means super unreliable connections, constant disconnects. Giving the device the SAS token is more secure because if the device generates the SAS token it requires the keys to the kingdom, if you give the device the SAS token then you can have it expire without the device knowing how to simply generate a new one

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.

2 participants