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

MQTT TSL fingerprint ignored #2033

Closed
wants to merge 1 commit into from
Closed

MQTT TSL fingerprint ignored #2033

wants to merge 1 commit into from

Conversation

lobradov
Copy link

Just figured out that Tasmota ignores MQTT TLS fingerprint and continues connection:

00:00:03 MQT: Verify TLS fingerprint...
00:00:04 MQT: Insecure connection due to invalid Fingerprint
00:00:06 MQT: Attempting connection...
00:00:06 MQT: Connected

To add insult to injury, message about invalid fingerprint is not at INFO loglevel, but only at DEBUG which is not on by default, so it's easily missed.

I added a message to INFO and broke the connection loop to retry. Not sure if this is the best way to handle it so please propose better one.

@arendst
Copy link
Owner

arendst commented Feb 27, 2018

In next pre-release

@arendst arendst closed this Feb 27, 2018
arendst added a commit that referenced this pull request Feb 27, 2018
5.12.0d
 * Prep for optional MQTT drivers by separating mqtt code from
sonoff.ino to file xdrv_00_mqtt.ino
 * Add compiler check for stable
lwIP version v1.4 (#1940)
 * Add diacritics to Polish language file
(#2005)
 * Add Hungarian language file (#2024)
 * Fix MQTT TLS
fingerprint validation (#2033)
@issacg
Copy link
Contributor

issacg commented Feb 28, 2018

I'd actually noticed this a month or so ago and thought this was intentional, and I don't like the new behavior as-is, for a number of reasons:

  1. It doesn't allow an up-to-date upgrade path to allow switching the certificate (this breaks backwards compatibility, and is the primary reason I'd argue to not leave the new patch as-is)
  2. It doesn't match the security of the OTA mechanism.

I'd suggest a patch which allows for 2 fingerprints (or 2 MQTT server configurations), similar to the WiFi config, and also patching the OTA mechanism (and allowing 2 fingerprints). This would mean a bit more RAM (~200 bytes) to hold these 3 variables, but would allow for real TLS validation, something which I haven't really felt as of yet. I'm willing to start working on this now, but wanted to get this comment out there.

@issacg
Copy link
Contributor

issacg commented Feb 28, 2018

OK, maybe I'm getting in over my head by taking this on my own, as there seems to be quite a lot of touchpoints needed to add something to Settings beyond the more simplistic patch of just falling back between fingerprints, but the idea still stands

@davidelang
Copy link
Collaborator

davidelang commented Mar 2, 2018 via email

@issacg
Copy link
Contributor

issacg commented Mar 4, 2018

@arendst Any chance you could help me by letting me know what touchpoints I need to deal with to propose a patch that would support the 2 fingerprints? I haven't found any commits (yet) that would provide a clean example of what needs to be changed. For example, I would assume beyond adding the SYSCFG entry it needs to be added to user_config, and probably to the commands for set/get, but where else? Also, how does one go about updating the memory address helpers you have in the SYSCFG struct?

Thanks!

issacg added a commit to issacg/Sonoff-Tasmota that referenced this pull request Mar 7, 2018
arendst added a commit that referenced this pull request Mar 9, 2018
5.12.0e
 * Add a second TLS fingerprint to allow switching keys in TLS
mode (#2033, #2102)
curzon01 pushed a commit to curzon01/Tasmota that referenced this pull request Sep 6, 2018
5.12.0d
 * Prep for optional MQTT drivers by separating mqtt code from
sonoff.ino to file xdrv_00_mqtt.ino
 * Add compiler check for stable
lwIP version v1.4 (arendst#1940)
 * Add diacritics to Polish language file
(arendst#2005)
 * Add Hungarian language file (arendst#2024)
 * Fix MQTT TLS
fingerprint validation (arendst#2033)
curzon01 pushed a commit to curzon01/Tasmota that referenced this pull request Sep 6, 2018
5.12.0e
 * Add a second TLS fingerprint to allow switching keys in TLS
mode (arendst#2033, arendst#2102)
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.

None yet

4 participants