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

Status WPS and SmartConfig #6680

Closed
arendst opened this issue Oct 18, 2019 · 27 comments
Closed

Status WPS and SmartConfig #6680

arendst opened this issue Oct 18, 2019 · 27 comments
Labels
fixed Result - The work on the issue has ended

Comments

@arendst
Copy link
Owner

arendst commented Oct 18, 2019

Can anyone verify if WPS is still working with pre-2.6.0? I thought I noticed it fails.

Do we still need WPS as an initial config tool?

@arendst arendst added the awaiting feedback Action - Waiting for response or more information label Oct 18, 2019
@Jason2866
Copy link
Collaborator

Jason2866 commented Oct 18, 2019

I do not have a router with WPS...
I cant remember anyone asking in Discord about WPS for setup
IMHO WPS can and should be dropped.
WPS has security issues https://www.howtogeek.com/176124/wi-fi-protected-setup-wps-is-insecure-heres-why-you-should-disable-it/

@ascillato
Copy link
Contributor

I can't test either and I agree with Jason

@andrethomas
Copy link
Contributor

I don't think I've ever actually had an interaction with someone who uses WPS.

@Jason2866
Copy link
Collaborator

I would suggest to drop the sonoff-classic build completly

@effelle
Copy link
Contributor

effelle commented Oct 18, 2019

WPS is considered one of the major vulnerability in a router since 2013 and many devices never got an update to fix or patch it in some way.
I can't test it since I use DD-WRT on my routers and is not present at all. Besides this, please consider WPS is deprecated and was replaced by Wi-Fi Easy Connect.

@ascillato2 ascillato2 added the poll Type - Poll label Oct 19, 2019
@blakadder
Copy link
Collaborator

Please kill WPS with fire and never mention it again

@arendst
Copy link
Owner Author

arendst commented Oct 22, 2019

With this feedback I think it's safe to say let's get rid of WPS.

I would also like to get rid of SmartConfig. It is default also only available in sonoff-classic and needs a phone with esp8266 app. I must say I haven't used it for over two years.

@arendst arendst changed the title Status WPS Status WPS and SmartConfig Oct 22, 2019
@blakadder
Copy link
Collaborator

That one also belongs in the "why the hell is this taking up space" category.
It is easier to use a phone and a browser instead of searching for and downloading the app to configure your tasmotized thingie, it just an unnecessary feature in 99% of the cases

@effelle
Copy link
Contributor

effelle commented Oct 22, 2019

I think I used SmartConfig just a couple of time and I don't miss it at all.

@arendst
Copy link
Owner Author

arendst commented Oct 22, 2019

It doesn't use any space on non sonoff-classic binaries as it is commented out.

The main issue is that it probably doesn't work anymore on the latest cores and I do not want to waste time on it to make it work again...

@arendst
Copy link
Owner Author

arendst commented Oct 22, 2019

What the hack. I remove it from the current release. If anyone wants to install tasmota using WPS or SmartConfig they need to use release 6.6.0 Afterwards they can always upgrade to the next releases.

Working...

arendst added a commit that referenced this issue Oct 22, 2019
 * Remove support for WPS and SmartConfig in favour of Web server (!) based WifiManager (#6680)
 * Remove binary sonoff-classic (#6680)
@arendst arendst added fixed Result - The work on the issue has ended and removed awaiting feedback Action - Waiting for response or more information poll Type - Poll labels Oct 22, 2019
@arendst
Copy link
Owner Author

arendst commented Oct 22, 2019

Done.

@arendst arendst closed this as completed Oct 22, 2019
@tobox
Copy link

tobox commented Oct 24, 2019

I once bought Airconditioning-WiFi-Modules from Mitsubishi, and they only supported WPS (the devices have just 2 Buttons - reset + WPS). It was a PITA to get them working in a OpenWRT-Only environment. I ended up connecting a third party router with proper WPS only to get the password into the devices. So yes, please remove WPS from Tasmota and add cool features with the free space!

@issacg
Copy link
Contributor

issacg commented Nov 20, 2019

I hate that I'm late to the party, but wanted to drop my $0.02 anyway...

Yes, WPS is not the most secure method (at least, not using the PIN - but I personally use the button, not the PIN). But it's not significantly worse than sending your WiFi credentials over HTTP on a WiFi connection that's secured - at best - with WPA2, which is also considered by many to be insecure - especially in the light of WPA3, which isn't supported by ESP chips (that I know of, anyway).

So rather than have a simple push-button way of zero-configuration for devices, users must either build their own firmware or else use another non-secure way to configure the WiFi that is, arguably, not as non-secure as WPS, but is certainly nowhere near as push-button.

I also note that with a single late exception, everyone on this thread so far is an active contributor to the project (and most also use non-standard router firmware, too). You're all "power users" presumably comfortable compiling your own firmware, so hardcoding your SSIDs is fine, but please remember that not everyone is so.

@meingraham
Copy link
Collaborator

Wi-Fi Manager is simple and secure and suitable for all types of user expertise. It's built in to all pre-compiled binaries. It does not preclude being able to use newer security protocols... once they are part of the Arduino Core.

@issacg
Copy link
Contributor

issacg commented Nov 20, 2019

I don't agree that Wi-Fi Manager is a perfect solution. For example, Wi-Fi manager isn't an option when the webserver is disabled, meaning that MQTT_TLS users can't use it either...

@meingraham
Copy link
Collaborator

We're speaking about the first time the device is to be configured if you are using a binary that is "generic" and does not contain the credentials is needs to connect to the Wi-Fi network. For example, the pre-compiled binaries, the web server is not disabled. If a user employs one of these binaries, the device will indeed broadcast an AP for three minutes after it powers up. During this window of time, another device could connect to the Tasmota device and take control by opening 192.168.4.1. Wouldn't the same be the case if the Tasmota device is in WPS mode and there is a malicious router that would pair to the device during that window of time? I think the opportunity for a malicious character to be looking for either a Tasmota AP (tasmota-xxxx) or deploys a device open to accept WPS requests are both as likely and presents equal risk to the user.

If, instead, a user has created a binary where they have disabled the web server, then now you are referring to a user that indeed has crossed that expertise threshold where they are able to compile their own firmware. In this case, the user has the option to specify every networking setting they care to set: Wi-Fi, TLS, Web UI password, etc.

@issacg
Copy link
Contributor

issacg commented Nov 21, 2019

There are plenty of cases where a generic MQTT hostname could be known up front, and with the new TOFU support for fingerprints, that can mean a generic binary.

I didn't intend to get caught up into comparing specific threat models - my point in mentioning it in the original point was that I didn't consider removing WPS a significant security-improvement, since much of the initial talk on this issue seemed to be focused around that.

@arendst has put so much thought over the past two years into keeping the configuration out of the firmware as much as possible - to be later configured and persisted in memory, separate from the code - and this just seems such a huge step backwards given, for example as I mentioned before, anyone using MQTT_TLS is now required to hardwire their SSID setup in the firmware, whereas before that wasn't required. And this just after 6.6.0.3 when the AWS IoT credentials were made configurable at runtime, and when TOFU allowed for simpler runtime configuration of fingerprint based TLS - things that were among the last little pieces allowing for more generic firmware to be used for TLS use-cases...

@effelle
Copy link
Contributor

effelle commented Nov 21, 2019

@issacg:
Issac, thanks for your feedback.
You right, the majority of the replies you saw on this topic are from contributors or devs .
At least for me, the decision was made considering what I see on our discord: I can't even remember a single user asking for WPS.
As @meingraham said, on first run a Tasmota device will be secured using AP mode, then security is covered anyway.
Finally, I completely understand your point of view but it is good to remember @arendst stated it was not working anymore on pre-2.6.0, thus was removed.

@s-hadinger
Copy link
Collaborator

anyone using MQTT_TLS is now required to hardwire their SSID setup in the firmware, whereas before that wasn't required

What makes you think that?

@issacg
Copy link
Contributor

issacg commented Nov 21, 2019

What makes you think that?

That's the million dollar question that I wish you'd asked yesterday. You're right, of course. In my upgrade path I glanced at the normal #ifdef check at the bottom of the settings header file and assumed that it said the same thing that it's always said

As to not be overly cryptic to anyone looking at this later, until 6.6.0 (I guess when AxTLS was swapped with BearSSL, it was indeed impossible to #define USE_WEBSERVER and #define USE_MQTT_TLS at the same time. As such, my user_config_override.h had #undef USE_WEBSERVER in it (as a necessity for MQTT TLS support). Since the two are no longer mutually exclusive, I can indeed use the Wi-Fi Manager option.

Big thanks to you @s-hadinger for your comment.

I'll freely admit that having Wi-Fi Manager makes the lack of WPS into a minor annoyance, but not worthy of the harsher tone implied by my previous comments, and apologize if my lack of due diligence caused offence to anyone.

@effelle
Copy link
Contributor

effelle commented Nov 21, 2019

Offence? None taken.
Is always good listen and compare different point of views.

@s-hadinger
Copy link
Collaborator

@issacg Now I understand.

Indeed swapping from AxTLS to BearSSL (and aggressive optimization), TLS is now a viable option. If you're interested, I put lots of details here: https://github.com/arendst/Tasmota/wiki/TLS

@issacg
Copy link
Contributor

issacg commented Nov 21, 2019

@s-hadinger I saw the Wiki page. I've been using TLS with fingerprints for a while. I actually found out about BearSSL and the new fingerprints the "hard way" too, just before discovering that when I hard-reset my test device to test the new fingerprints I also nuked my Wi-Fi setup 😁

I was admittedly a bit unhappy with the breaking fingerprints, and moreso with the lack of alternative to the new TOFU setup with the very customized fingerprint, rather than some fingerprint that would be easier to calculate with standard openssl tooling. I agree that fingerprinting the public key makes sense in a setup like LetsEncrypt where the certificate is silently and automatically rotated, yet on the other hand I didn't understand the logic, in that LetsEncrypt had already been supported by #define USE_MQTT_TLS_CA_CERT (which disappeared, too, I think?) It's just not the sort of major breakage that I'd expect from a non-major-version release, but maybe that's just me...

Regardless, in that case, I tried to rectify that with a tool that I published and added a mention of said tool to that Wiki page, so hopefully I've made other folks' lives a bit easier.

But the Wiki page was instrumental for me to understand the changes, so kudos for writing it!

@s-hadinger
Copy link
Collaborator

s-hadinger commented Nov 21, 2019

Breaking changes are never a good thing. The fingerprint change was mainly motivated by the fact that BearSSL works differently and fingerprinting the public key instead of the certificate allowed to save significant memory and flash size. And it makes sense too.

Now if you are using Letsencrypt, just use #define USE_MQTT_TLS_CA_CERT. The Letsencrypt CA Certificate is already included and you don't need fingerprints.

The embedded CA is:
LetsEncrypt IdenTrust DST Root CA X3 certificate, RSA 2048 bits SHA 256, valid until 20210417

@issacg
Copy link
Contributor

issacg commented Nov 21, 2019

Now if you are using Letsencrypt, just use #define USE_MQTT_TLS_CA_CERT. The Letsencrypt CA Certificate is already included and you don't need fingerprints.

So it is still there. Might be worth reverting to the old comment that explained that it was specifically for embedding the LetsEncrypt cert. If I hadn't known about that from past versions, I'd have never known it did so (I know it's in the Wiki page, but personally for me it's more helpful for the explanation to be "in my face" while I'm looking at all of the tunables)

@s-hadinger
Copy link
Collaborator

Good point. In my Todo list unless you are willing to do a PR to change the comments in the config source file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed Result - The work on the issue has ended
Projects
None yet
Development

No branches or pull requests