Add URL whitelisting capability (accessible for many integrations)

keiranharris commented 4 years ago

Context

The specific problem that triggered this thinking is, after v0.107.7 attempts to post an image into slack from an "external" URL - (well actually its internal on my LAN - a security camera that it snaps a still image from - and i trust that URL - but hass doesnt know that) generates a TypeError (see issue #36947 for a more detailed breakdown).

It seems there was a deliberate architectural decision (attn: @balloob) to kill this functionality based on security grounds (see this comment for further info). And it seems from that comment, its not just Slack, other integrations were also targeted, hence all could benefit from development to re-enable.

Going forward, this functionality (i.e. pull an image from an URL that you have deliberately deemed safe prior to the fact, and allow posting to XYZ in later parts of the automation) should be re-enabled again, but in a secure way.

Proposal

The change i am proposing is to to add URL whitelisting capability. To allow the user to pre-determine which URL's are safe to retrieve data from (and implicitly denying all others).

Consequences

This will allow the posting of images in a SECURE way to components such as slack again.

balloob commented 4 years ago

Sounds good 👍

bachya commented 4 years ago

We can mimic what whitelist_external_dirs does. Since I’m the codeowner for Slack (where there’s a specific interest), I’ll take a crack at it.

bachya commented 4 years ago

@balloob Should we allow for any “automatically approved” URLs (like those on private IP ranges)? Or follow whitelisted_external_dirs and only allow those URLs that are explicitly defined?

(Trying to think of a use case where explicitly spelling out URLs would be a pain for the user...)

balloob commented 4 years ago

We should only allow explicitly defined. Private IPs are the security problem because other sources might allow unauthenticated requests from internal network.

keiranharris commented 4 years ago

Guys, this is my first REAL exposure to the beauty of crowd sourcing dev and just how powerful git is. To go from me noticing the ‘problem’ last weekend, and you guys 1/ pointing me to a well documented architecture decision 2/ allowing me to raise an issue against that architecture 3/ the main hass guy approving the change 4/ the owner of the slack integration doing a PR to make the change 5/ having it merged back to mainline code...... all in under a week, to say im impressed is quite the understatement! Thanks both of you for actioning my little problem, very appreciative here. Keiran.

bachya commented 4 years ago

@keiranharris Awesome! Thank you for giving me the nudge. 🙏🏻

(P.S., will be implementing this for Slack very soon.)

momelod commented 4 years ago

@keiranharris Awesome! Thank you for giving me the nudge. 🙏🏻

(P.S., will be implementing this for Slack very soon.)

looking forward to it.

keiranharris commented 3 years ago

Hey @bachya - checking back in on this, on the weekend ill have some free time to upgrade hass > v0.107.7.... did this feature get rolled in? I checked the slack integration webpage and there is mention of whitelisting now, so am i safe to go? (What version of mainline did it get included if so). Thanks mate!

bachya commented 3 years ago

@keiranharris You bet! This was released in 0.113 – if you upgrade to the latest (0.115.6 [or the 0.116 beta if you'd like]), you'll get this. 👍

home-assistant / architecture