Closed salim-b closed 5 months ago
Besides, I'm wondering whether we should limit the mounts (map
in config.yaml
) by
ssl
read-only, i.e. remove the :rw
ssl
. Why does Syncthing need access to custom certs in the first place? We have Ingress for that, no?addons:rw
. What is the use case for Syncthing having access to local add-ons? Not even the official File editor add-on has access to addons
since it's deemed confusing for non-developers.After the change the default configuration will just work for most of the users. I would keep all the folder just in case if someone needs it.
- it is confusing to have so many folders. I cannot find any documentation what is the purpose of each of them. It is hard at the beginning to understand which one is right to use
Yeah, I feel the same. The official add-on development documentation only states about the map
config key:
List of Home Assistant directories to bind mount into your container. Possible values:
homeassistant_config
,addon_config
,ssl
,addons
,backup
,share
,media
, andall_addon_configs
. Defaults toro
, which you can change by adding:rw
to the end of the name.
The documentation of the official Samba add-on gives some more info about the purpose of these folders:
Directory Description addons
This is for your local add-ons. backup
This is for your snapshots. config
This is for your Home Assistant configuration. media
This is for local media files. share
This is for your data that is shared between add-ons and Home Assistant. ssl
This is for your SSL certificates.
Note that config
has been deprecated in favor of the new, more granular homeassistant_config
, addon_config
and all_addon_configs
directories.
- On the other hand samba seems to expose all of them so maybe there is the reason to have access to those https://github.com/home-assistant/addons/blob/133709534a0bc853ee93a2315657b59caa67ca0e/samba/config.yaml#L18
The Samba add-on has access to all of these folders to provide users with a convenient way to edit config files, not because Samba itself needs access.
I would keep all the folder just in case if someone needs it.
I'd rather limit access to only the folders for which there's an actual Syncthing use case, i.e. backups
, media
and share
. This would be less confusing for users and better security-wise.
I'd rather limit access to only the folders for which there's an actual Syncthing use case, i.e.
backups
,media
andshare
. This would be less confusing for users and better security-wise.
syncthing can be also used to check/edit configuration like smb. You could just sync ha config, change it locally and reload.
btw is migration script needed? If it stop working after update it will be pretty annoying.
syncthing can be also used to check/edit configuration like smb. You could just sync ha config, change it locally and reload.
Yeah, I thought about such a use case. But honestly, I don't think it is a good idea. Way too easy to accidentally delete your HA config. People should just use the official File editor or Studio Code Server add-ons or SSH into HA to directly edit config files.
Plus, mapping config
/all_addon_configs
also means it's very easy for users to accidentally expose sensitive stuff to their Syncthing remotes. To cite the blog post announcing the new addon_config
mount:
Add-ons that map
config
have far more access than they should sinceconfig
includes all secrets and credentials used in your Home Assistant integrations.
btw is migration script needed? If it stop working after update it will be pretty annoying.
Ideally yes. But I didn't look into it yet (didn't even test the PR yet).
I think all we need to do for proper migration is to check if there's a /data/config
folder before launching Syncthing and if yes,
move config.xml
, cert.pem
, key.pem
, https-cert.pem
and https-key.pem
to /config/
, and
move everything else to /data/
(i.e. one directory level up).
Update: See https://github.com/Poeschl/Hassio-Addons/pull/450/commits/289da16e3212ddd25febf57f695c7059480b3e65
Thanks a lot guys for making those changes. I hope I can find some time this week to go through the MRs :heart_decoration:
Due the merge conflicts I patched the changes manually ;) See #452
Due the merge conflicts I patched the changes manually ;) See #452
@Poeschl Cool, thanks!
Did you actually test the changes by this PR? Because I didn't... 😬
@Poeschl I just updated my HA instance to the latest add-on version 1.18.0 and noticed the config migration didn't work as expected because of an unsupported mv
CLI flag, see https://github.com/Poeschl/Hassio-Addons/pull/453.
This means the 1.18.0 version breaks people's Syncthing config. Merging #453 should resolve it, so please have a look ASAP. Thanks!
Changes:
Use environment variables instead of CLI arguments where possible and properly separate config and database/state directories (related to https://github.com/Poeschl/Hassio-Addons/issues/447#issuecomment-1872160368).
With this change, the add-on now mounts the new
addon_config
instead ofconfig
and stores Syncthing's/config
which maps to/mnt/data/supervisor/addon_configs/243ffc37_syncthing/
on the host (I think?)./data
which maps to/mnt/data/supervisor/addons/data/243ffc37_syncthing/
on the host.Set
HOME=/share
as default folder path fallback, thus fix https://github.com/Poeschl/Hassio-Addons/issues/447.Slightly improve the logging in the s6 run script.
Avoid creating a default folder when generating an initial configuration / starting for the first time by setting
STNODEFAULTFOLDER=1
. Syncthing shouldn't actually attempt to create a default folder since we setSTCONFDIR
andSTDATADIR
to paths that already exist. But in case anything goes south,STNODEFAULTFOLDER=1
ensures we don't start writing to arbitrary paths.Note that I haven't actually tested this PR yet 😬