search

mdegat01 / addon-promtail

Promtail for Home Assistant
MIT License
15 stars 18 forks source link

Access /config directory #185

Closed eroldan closed 2 years ago

eroldan commented 2 years ago

First, thank you for bringing up Loki to Home Assistant. I'm trying to ingest some syslog streams with promtail so I would like to add an additional scrape config. I find counterintuitive that the addon does not have /config mapped. Probably read-only is enough to hold /config/promtail/syslog.yaml or similar. I don't really know if there is some official recommendation on using /share over /config. For me is more intuitive to have these kind of files in /config (just as esphome, appdaemon, etc)

Thanks!

mdegat01 commented 2 years ago

So the problem with mapping /config is that's really asking for a lot of access. Even read-only access means this addon will be able to see every secret in your secrets.yaml file, all the auth/credentials information HA stores for UI-configured integrations in .storage and so much more. There's just a ton of sensitive information stored unencrypted in /config. That's why I choose not to map it and feel like (personally) addons should only map it if absolutely necessary. Like if providing a service which actually requires touching HA config or interfacing more directly with HA's config.

If I could provide users an option to say whether they wanted to map /config or only map a specific folder within /config I would do that. But the only way I can do what your asking is change the addon config to map all of /config for all users of the addon and I would prefer not to do that.

That being said, I can suggest a simple local change that will make it appear like your promtail config is in /config without it actually being there. ln lets you make links between directories. So if you ssh into HA you can do this:

ln -s /share/promtail /config/promtail

That should make it appear like everything is in /config from wherever you edit config without actually giving this addon access to all that.

eroldan commented 2 years ago

Thanks for reviewing this. I totally understand what you say, and in the current status of Home Assistant may make sense. But also let me dissert and share my view: The problem of overloading /config with diverse credentials accessible by many actors is already there. It may be HA core developers duty to compartment each installed component with SELinux, ACL or whatever technology and let the user open the gates. Currently, seems the design is to let the addon developer to "harden" their app using AppArmor (which I'm not familiar with) . Making a quick look seems doable to ban read on all /config but permit /config/promtail/* .

https://developers.home-assistant.io/docs/add-ons/presentation#apparmor

I will read a bit more about this.

mdegat01 commented 2 years ago

Yea I was thinking about that. I already have a custom apparmor profile in this addon so that's actually a pretty simple change. You can see it here: https://github.com/mdegat01/addon-promtail/blob/main/promtail/apparmor.txt . Basically just add /config/promtail/** r to the promtail profile.I actually considered doing that with share and ssl as well but it felt less risky with those since those folders were designed to be shared among addons.

I could get on board with this approach. Ideally should also add a regex to the config to ensure the provided file path is one that will work. Which is probably a good idea anyway since it just asks for a string right now.

eroldan commented 2 years ago

Awesome. I will look forward to try your next release. Although there isn't mentioned in documentation ( https://developers.home-assistant.io/docs/add-ons/security#making-a-secure-add-on ), there is some valid logic in having (ssl,share,config)/{addon_name}/** defined by default in apparmor profile, if there isn't a more explicit requirement.

github-actions[bot] commented 2 years ago

There hasn't been any activity on this issue recently, so we clean up some of the older and inactive issues. Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by leaving a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thanks!