Skip to content

Add smb encrypt option #4289

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.

Sign up for GitHub

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

Draft
7CqKcKvfAf wants to merge 2 commits into
base: master
Choose a base branch
from
+30 −3
Draft

Add smb encrypt option #4289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
044015c
Add smb encrypted option
7CqKcKvfAf Jan 11, 2026
8560adf
Enforce server signing when encryption is required
7CqKcKvfAf Jan 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions samba/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## 12.5.5

- Add option for setting smb encrypted parameter
Copy link
Copy Markdown
Member

@agners agners Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a note that the default changes.


## 12.5.4

- Fix invalid inverted commas in server signing parameter
Expand Down
7 changes: 7 additions & 0 deletions samba/DOCS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,13 @@ This can cause issues with file systems that do not support xattr such as exFAT.

Defaults to `true`.

### Option: `encryption`

Configure the SMB encryption requirement. This option encrypts all traffic between client and server and prevents guest access if set to required.
Refer to the man page for smb.conf for detailed information about the values: **off**, **desired** and **required**.

Defaults to `desired`.

### Option: `server_signing`

Configure the SMB server signing requirement. This option can improve security by requiring message signing, which helps prevent man-in-the-middle attacks.
Expand Down
4 changes: 3 additions & 1 deletion samba/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
version: 12.5.4
version: 12.5.5
slug: samba
name: Samba share
description: Expose Home Assistant folders with SMB/CIFS
Expand Down Expand Up @@ -37,6 +37,7 @@ options:
- ssl
compatibility_mode: false
apple_compatibility_mode: true
encryption: "desired"
Copy link
Copy Markdown
Member

@agners agners Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Samba 4.18 smb.conf man the default is if_required. Is there a reason you choose desired here? I fear that this might cause existing setups to break.

Also, smb.conf also knows the default string, maybe this is the safest choice, as it relies on the Samba maintainers judgment what is the best default.

Copy link
Copy Markdown
Contributor

@krikk krikk Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default == if_required: will only enable encrytion if the client requires it
desired: will always enable encryption, except in the case the client does not support it

so desired is better because it encrypts all possible connections, while still supporting clients which can't encrypt

...and this is a server service, which should be shipped with "secure" defaults, SMB 3 was introduced with Windows 10 / Server 2016, so every supported windows Client today will support SMB Encryption, if one need access with older clients, he can still use compability mode

Copy link
Copy Markdown
Member

@agners agners Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why the Samba developers did not choose this option by default?

Copy link
Copy Markdown
Contributor

@krikk krikk Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the developer of this addon is setting up a smb-server, as such he is indirectly the admin of a samba server, and as admin he is responsible do setup the server with secure defaults, because the end-user who is installing this addon/app does not care/does not know how to do a secure setup...

running unencrypted services, even on LAN is NOT secure and should not be embraced by unsecure defaults, especially as there are practically no more clients which do not support smb encryption, even Windows 8 and Windows Server 2012 do support it, also all smbclients on linux support it

.../me is voting for secure defaults ...but this are just my 2 cents :)

Copy link
Copy Markdown
Member

@agners agners Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I get that you want to pick a good, secure default. I generally agree with that principle.

It just strikes me as odd that this is not the default in Samba 🤔Makes me wonder if there are reasons 🤷 .

I way more towards least surprise: This PR as it stands today changes the default for existing users. At the very least, this should be made clear in the changlog.

Also, no matter what default we are going with, at the very least let's add the old default if_required, and I'd also add default as an option as well.

Copy link
Copy Markdown
Author

@7CqKcKvfAf 7CqKcKvfAf Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should provide the end user options which make it easy to understand what will be the achieved effect. This is why I decided to only expose off/desired/required as global options.

Concerning the default value: you can imagine I was a little bit surprised when I observed local network traffic between my linux client and HA server with wireshark and found out that I was able to ready everything in plaintext.

If we provide default as option, many people might just use it without reading smb.conf documentation, not knowing that encryption will not be enabled on many or most connections. Personally I often trust (or hope) that developers/publishers pick safe config defaults.

Furthermore I think it would be good if the user could supply his own smb.conf to the app without being forced to build a custom HA app. But that is a little bit offtopic.

Copy link
Copy Markdown
Member

@agners agners Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we provide default as option, many people might just use it without reading smb.conf documentation, not knowing that encryption will not be enabled on many or most connections.

If that is the concern, then let's add a quick note in the option description: "Note: default will only encrypt if clients require encryption."

Not having today's default as a possible option is a no-go from my point of view. At the very least it is useful to regression test in case something doesn't work the way users expect after upgrading the Samba app to this new version.

server_signing: "default"
veto_files:
- ._*
Expand All @@ -60,6 +61,7 @@ schema:
- "match(^(?i:(addons|addon_configs|backup|config|media|share|ssl))$)"
compatibility_mode: bool
apple_compatibility_mode: bool
encryption: list(off|desired|required)
Copy link
Copy Markdown
Member

@agners agners Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation of Samba 4.18 (as shipped with the current app version) lists if_required as well (see smb.conf man). Maybe it also makes sense to add default (see above).

server_signing: list(default|auto|mandatory|disabled)
veto_files:
- str
Expand Down
12 changes: 10 additions & 2 deletions samba/rootfs/usr/share/tempio/smb.gtpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,20 @@
interfaces = lo {{ .interfaces | join " " }}
hosts allow = 127.0.0.1 {{ .allow_hosts | join " " }}

smb encrypt = {{ .encryption }}
Copy link
Copy Markdown
Member

@agners agners Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To align with the other config uses:

Suggested change
smb encrypt = {{ .encryption }}
server smb encrypt = {{ .encryption }}

{{ if eq .encryption "required" }}
client min protocol = SMB3
client max protocol = SMB3
Copy link
Copy Markdown
Member

@agners agners Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client features are not used by the add-on so this doesn't really do anything.

server min protocol = SMB3
Copy link
Copy Markdown
Member

@agners agners Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? With smb encrypt From smb.conf man it sounds like this is anyways required when smb encrypt is set to required

server max protocol = SMB3
server signing = mandatory
{{ else }}
server signing = {{ .server_signing }}
{{ if .compatibility_mode }}
client min protocol = NT1
server min protocol = NT1
{{ end }}
{{ end }}

mangled names = no
dos charset = CP850
Expand All @@ -31,8 +41,6 @@
vfs objects = catia fruit streams_xattr
{{ end }}

server signing = {{ .server_signing }}

{{ if (has "config" .enabled_shares) }}
[config]
browseable = yes
Expand Down
6 changes: 6 additions & 0 deletions samba/translations/en.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ configuration:
Enable Samba configurations to improve interoperability with Apple
devices. May cause issues with file systems that do not support xattr
such as exFAT.
encryption:
name: Encryption
description: >-
Configure SMB encryption.
SMB3 protocol is used if set to required.
Copy link
Copy Markdown
Member

@agners agners Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be interpreted as if SMB3 is not used if this is not set to required. But that is not the case: "SMB protocol takes care of choosing the appropriate protocol." so SMB3 might be used even without this.

Compatibility mode option will be ignored in this case.
server_signing:
name: Server signing
description: >-
Expand Down