Cleanup and optimize code

[enwi]

Changes:

• Reformat code (also remove tabs)
• Move includes to top of files
• Use constants instead of defines
• Use initializer list instead of direct assignment
• Rearrange function definitions like in header file
• Use doxygen documentation for already documented functions
• Skip http header with client.find("\r\n\r\n"); instead of complicated loop
• Do not store body in string, when it can be read by ArduinoJson directly (#7)
• Remove unneeded nesting by changing if statements
• Add missing return statement in checkUpgrade() and doUpgrade()
• Reduce complexity and number of intermediate variables in _urlDetails(...)
• Reuse urlDetails_t in _resolveRedirects() to reduce number of intermediate variables
• Reduce size of needed JsonDocument from >5000 bytes to 256 bytes plus 32 bytes filter to reduce heap usage
• Use client.printf_P to reduce heap usage

Also closes #7

[yknivag]

Hi @enwi - many thanks for your interest in this project, it's something I need to get back to, if only to regenerate the certs file.

I'll review your proposal in more detail shortly, but I'm afraid I can't accept a PR which contains client.setInsecure();. Yes it is more "efficient" but this library was designed to work securely.

client.setInsecure() leaves open the risk of a MITM attack, and whilst that may be of little consequence in some projects for updating firmware it poses a significant risk. Without the proper SSL checks it would be very easy for an attacker to force the redirect cycle (a nuisance imposed by GitHub) to a different URL and to unintended firmware. Given that there is no firmware signing, it would be very difficult to detect that.

If the library isn't working in the secure mode then there are instructions in the readme for how to regenerate the certificate bundle which should fix that.

Kind Regards,

Gavin.

[enwi]

Hey @yknivag I knew I forgot to remove some of my test code. I had the client.setInsecure(); in there, because I tested sth. I am aware of the possibility of MITM when not using TLS.

A little more background: I am facing the issue that the redirect by GitHub to the proper binary file for some reason errors, because the client can't connect to objects.githubusercontent.com. That's why I just left out the TLS part to make sure it's not the certificate causing me trouble, but that's not the case. Might need to open another issue, so we can take a deeper look in what's going wrong. (see #9)

[yknivag]

Hi @enwi - thanks for opening the other issue. I've made an initial reply there.

For this PR I will look to merge as soon as I have tested - it all looks very sensible.

Thank you once again for taking the time to work on the code.

Gavin.

[enwi]

@yknivag I have made some more changes and fixed my own error I introduced

[yknivag]

Hi @enwi you've done so much work with this - thank you!

I see you have closed Issue #9 - does that mean that the code in it's current form is fully working?

Gavin.

[enwi]

You are welcome 🙂

Yes I have tested it myself and confirmed it working, but I would suggest you take a deep look at my changes and also test it again.

[enwi]

I have some more changes that also reduce the needed memory (heap size), which also fixes an error for me with the received json from GitHub (if it is too large, it is possible that the json cannot be parsed anymore)