Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Samba plugin does not allow managing shares exposure #3696

Open
as-kholin opened this issue Jul 23, 2024 · 11 comments · May be fixed by #3701
Open

Samba plugin does not allow managing shares exposure #3696

as-kholin opened this issue Jul 23, 2024 · 11 comments · May be fixed by #3701

Comments

@as-kholin
Copy link
Contributor

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 Samba: 'Veto files' will hide all files no matter the share #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
Copy link

OakLD commented Jul 27, 2024

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
Copy link
Contributor Author

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:

  • Users by share could be done by (UI) checkbox that exposes UN and Pass fields and (code) making the checkbox being checked add the additional user to the config, and set that as the share user instead of the generic user
  • access control could be accomplished by setting valid user to a dummy user not given SMB permissions, and adding Invalid user for the entire real user list.

@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
Copy link
Contributor Author

@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.

@as-kholin as-kholin changed the title SMB plugin does not allow managing Samba plugin does not allow managing Jul 29, 2024
@as-kholin as-kholin changed the title Samba plugin does not allow managing Samba plugin does not allow managing shares exposure Jul 30, 2024
Copy link

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.

@github-actions github-actions bot added the stale label Aug 29, 2024
@as-kholin
Copy link
Contributor Author

Still active, pending PR review

@github-actions github-actions bot removed the stale label Aug 29, 2024
Copy link

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.

@github-actions github-actions bot added the stale label Sep 28, 2024
@as-kholin
Copy link
Contributor Author

Still active, pending PR review by @frenck

@github-actions github-actions bot removed the stale label Sep 28, 2024
Copy link

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.

@github-actions github-actions bot added the stale label Oct 28, 2024
@as-kholin
Copy link
Contributor Author

Still active pending PR review.

@github-actions github-actions bot removed the stale label Oct 28, 2024
Copy link

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.

@github-actions github-actions bot added the stale label Nov 27, 2024
@as-kholin
Copy link
Contributor Author

Pending review of #3701

@github-actions github-actions bot removed the stale label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants