Skip to content

Remove Option to disable VERIFY_SSL #11195

Closed
ryanm101 wants to merge 19 commits into
home-assistant:devfrom
ryanm101:ssl_verify_remove
Closed

Remove Option to disable VERIFY_SSL #11195
ryanm101 wants to merge 19 commits into
home-assistant:devfrom
ryanm101:ssl_verify_remove

Conversation

@ryanm101
Copy link
Copy Markdown
Contributor

@ryanm101 ryanm101 commented Dec 18, 2017

Based on discussion here:
#11072

This is a 1st pass at removing the ability to disable SSL_VERIFY this will increase security by ensuring certificates are trusted and have been configured correctly.

@ryanm101 ryanm101 changed the title Ssl verify remove Remove Option to disable VERIFY_SSL Dec 18, 2017
@MartinHjelmare
Copy link
Copy Markdown
Member

Also needs documentation update. But I'd wait for a review from one of the members that requested this.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Dec 18, 2017

Most usage of that option is while people don't setup stuff correct. With dnsmasq you can easy solve external internal resolver problems or ssl proxy can make the life easier.

@ryanm101
Copy link
Copy Markdown
Contributor Author

ryanm101 commented Dec 18, 2017

@pvizeli I agree. However per my PR for the plex sensor, I'm keen to be consistent so if we are not going to allow the option going forward then imo it makes sense to do this cleanup so there is no confusion as to why am I able to do that in this component yet not in that one.

I'll do the docs updates today / tonight.

Supporting Issues / Related info / Potential Reasons not to do this:
Open:

Closed:

@ryanm101
Copy link
Copy Markdown
Contributor Author

@armills @balloob Could you guys please weigh in?

…SL was removed,

I hadnt done a proper pull to update so this was causing a conflict.
@fabaff
Copy link
Copy Markdown
Member

fabaff commented Dec 18, 2017

I'm strongly against that change. This doesn't increase the security as the validation is enabled by default but will break a lot of setups. For some appliances (e.g., the NAS sensor platform or the device tracker here) you need to run with a non-valid certificate because there is no way to change it.

A lot of DIY devices are using self-signed certificates as well and those devices could be integrated by the rest platforms. An increase in security would be if it's possible to specify a CA bundle in the configuration file. Hmmm, I doubt that it would help because it's an additional hassle to create a proper configuration of all involved parties to accept self-signed certificates for the average user. If we decide to go down that route then it needs to be a one-click solution for HA itself and Hass.io.

I agree that for third-party/web-based services or RESTful API it shouldn't be possible to disable the verification. The pvoutput sensor was done that way already.

@ryanm101
Copy link
Copy Markdown
Contributor Author

ryanm101 commented Dec 18, 2017

@fabaff

A lot of DIY devices are using self-signed certificates as well and those devices could be integrated by the rest platforms.

But solution to this is to simply import the Cert onto the hass server, at that point it will be trusted.

For some appliances (e.g., the NAS sensor platform or the device tracker here) you need to run with a non-valid certificate because there is no way to change it.

I dont have those appliances but what do you mean when you say invalid? it it expired or just self signed? and why isnt the solution to import the cert into the trusted certs for the server?

@fabaff
Copy link
Copy Markdown
Member

fabaff commented Dec 18, 2017

But solution to this is to simply import the Cert onto the hass server, at that point it will be trusted.

This involved additional steps on the command-line. It's even harder for a Hass.io user. You have to access the container directly and need to re-add it every time after you performed an upgrade.

Self-signed, expired, name mismatch, etc.

@ryanm101
Copy link
Copy Markdown
Contributor Author

You have to access the container directly and need to re-add it every time after you performed an upgrade.

The 1st point is more we just need to develop an interface, off the top of my head we should detect the mismatch and display a prompt to the user (similar to autodiscovery) if they want to trust the cert, if yes it imports. This would also solve the cmdline issue for non-hassio users

The 2nd point we should be able to do as we do with /config just store the certs on in the same datastore.

To be clear I'm not saying this is a simple task but I do think we should start looking at that as a solution.

While I was in preference of keeping the verify_ssl option the more i think about it the more I see it as a problem as a) we allow people to do the wrong thing and b) people doing this think they are secure when in fact they are actually in a worse position as they are now susceptible to MIM attacks while thinking they've secured stuff.

I'm no UI expert but I'll take a peek if we get a few more people weighing in on the feasibility of doing this.

@omribahumi
Copy link
Copy Markdown

omribahumi commented Dec 23, 2017

As the author of #11044, I'll add my 2 cents:
I asked to add support for disabling SSL verification on the Luci component. I don't feel the need to strongly secure the connection between my HA and my router. You can warn people about the dangers of disabling it, but please let me decide about my own network's threat models. I trust the Wifi authentication for now and strong SSL feels like an overkill in that particular case.

Perhaps you should only support this for LAN components and not for cloud based components.
Maybe even name it insecure_ssl to emphasize the danger.

@balloob
Copy link
Copy Markdown
Member

balloob commented Dec 24, 2017

there are per platform use cases to disable ssl. With Plex, there was no reason as we either go local without cert or via cloud with cert. I don't think that we should remove all.

@ryanm101 ryanm101 closed this Jan 3, 2018
@ryanm101 ryanm101 deleted the ssl_verify_remove branch March 25, 2018 19:51
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants