home-assistant / addons

:heavy_plus_sign: Docker add-ons for Home Assistant
https://home-assistant.io/hassio/
Apache License 2.0
1.57k stars 1.52k forks source link

Samba plugin does not allow managing shares exposure #3696

Open as-kholin opened 4 months ago

as-kholin commented 4 months ago

Describe the issue you are experiencing

The SMB plugin has no configurability on the shares exposed - It always exposes the same 7 folders.

The content in these folders ranges from the critically vital to not allow exposure (TLS certificates, core config and backups that may contain credentials), to the mundane (media files). There is no ability to expose less than that, or to use a separate account for the mundane vs. critical

The config should have two enhancements, though even just the first would be a world better than today:

  1. There should be checkboxes to allow users to selectively disable the default shares (for example - disable all but Media, so only media is exposed via SMB)
  2. An addition that would be even better - below those checkboxes, allow a user to then specify user-defined locations and share folder name for it. An example of where this would be nice is in #3551 , where the user is wanting to expose only subfolders of media - being able to define specific folders to expose allows further control and management for those who want to take on that complexity.

While there are security implications of point 2 for end users - I am not sure there is anything they can expose that would realistically be more sensitive/critical than the items already exposed by default.

What type of installation are you running?

Home Assistant OS

Which operating system are you running on?

Home Assistant Operating System

Which add-on are you reporting an issue with?

Samba share

What is the version of the add-on?

12.3.1

Steps to reproduce the issue

N/A - issue explained from plugin documentation

System Health information

Redacted System Information

version core-2024.7.3
installation_type Home Assistant OS
dev false
hassio true
docker true
user root
virtualenv false
python_version 3.12.4
os_name Linux
os_version 6.6.33-haos
arch x86_64
timezone America/Chicago
config_dir /config
Home Assistant Supervisor host_os | Home Assistant OS 12.4 -- | -- update_channel | stable supervisor_version | supervisor-2024.06.2 agent_version | 1.6.0 docker_version | 26.1.4 healthy | true supported | true host_connectivity | true supervisor_connectivity | true ntp_synchronized | true supervisor_api | ok version_api | ok installed_addons | Samba share (12.3.1), others redacted as irrelevant ### Anything in the Supervisor logs that might be useful for us? _No response_ ### Anything in the add-on logs that might be useful for us? _No response_ ### Additional information _No response_
OakLD commented 4 months ago

Agreed, I am trying to figure this out too. It doesn't seem to use standard /etc/samba/smb.conf and neither the smbpasswd command over ssh or ha> is found. I can't find the configuration file and if I find it, I don't think this addon allows for more than 1 user (since no sections are present) and there's no property to set folders to access.

Simple access to a /etc/samba/smb.conf and smbpasswd would resolve this issue for advanced users, who want to configure access to multiple users and shared folders.

as-kholin commented 4 months ago

While I would prefer that level of flexibility as well, I think it unlikely. The problem becomes that a template is being used to fill in variables(so a lot of it is hard coded), and SMB is running in a different container - so we are limited to what that container can see for config (for example, would not be seeing HA container directly, only shared folders assigned to both) - and best practice is that HA container should not be able to edit it.

That's good for preventing a Samba vulnerability from allowing direct attack on HA - other than the fact that most of the most critical parts of HA config are shared RW to Samba, so it's only really protecting the core container executables, not HA directly.

I suspect even subfolder access is going to be a PITA without rearchitecting completely.

With that said, a few points:

@OakLD - in case it helps, we can see the file it creates, just not locally on haos. In github:

So dev to fix would involve adjustments to smb.gtpl, the run script, the UI, and the JSON the UI creates.

as-kholin commented 3 months ago

@OakLD I looked at it for a bit, and realized the minimal level (allowing you to enable specific folders selectively) was a simple change with minimal UI issues. I submitted #3701 to address that minimal level.

From looking at it, the other parts are possible - setting share-specific credentials is doable, and adding 'sub-shares' is also a viable option - but neither can be done nicely via the GUI (which is already long, and had to be made longer to allow enablement) - so would probably need much larger changes in order to add those (or have those be YAML config).

Because of that, and because those would both materially increase the complexity and time to complete, I left those alone for the moment. If this commit goes well, I may circle back on those in the future.

github-actions[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

as-kholin commented 2 months ago

Still active, pending PR review

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

as-kholin commented 1 month ago

Still active, pending PR review by @frenck

github-actions[bot] commented 4 weeks ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

as-kholin commented 4 weeks ago

Still active pending PR review.