Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @7CqKcKvfAf
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
📝 WalkthroughWalkthroughAdds a new SMB encryption option (off|desired|required), bumps version to 12.5.5, updates schema, docs, translation, and templates to conditionally emit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
samba/rootfs/usr/share/tempio/smb.gtpl (1)
22-34: Consider usingserver signing = mandatoryinstead ofautowhen encryption is required.The logic correctly enforces SMB3 protocols when encryption is required and properly ignores compatibility_mode in this case. However, Samba best practices recommend
server signing = mandatorywhen using SMB3 with encryption enabled. While signing becomes implicit with encryption at the transport level, explicitly setting it to mandatory ensures consistent security enforcement and aligns with documented hardening guidance.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
samba/CHANGELOG.mdsamba/DOCS.mdsamba/config.yamlsamba/rootfs/usr/share/tempio/smb.gtplsamba/translations/en.yaml
🧰 Additional context used
📓 Path-based instructions (1)
*/**(html|markdown|md)
⚙️ CodeRabbit configuration file
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
*/**(html|markdown|md): - Use bold to mark UI strings.
If "" are used to mark UI strings, replace them by bold.
Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Use sentence-style capitalization also in headings.
do not comment on HTML used for icons
Avoid flagging inline HTML for embedding videos in future reviews for this repository.
Files:
samba/CHANGELOG.mdsamba/config.yamlsamba/DOCS.md
🔇 Additional comments (7)
samba/CHANGELOG.md (1)
3-6: LGTM!The changelog entry follows the established format and accurately describes the new feature.
samba/translations/en.yaml (1)
36-41: LGTM!The translation entry accurately describes the encryption feature and its interaction with compatibility mode.
samba/DOCS.md (2)
110-116: LGTM!The documentation is clear, direct, and follows the Microsoft Style Guide. UI values are properly bolded, and the explanation is concise.
117-122: LGTM!The documentation enhancement is clear and follows guidelines. The security explanation is valuable, and values are properly formatted.
samba/config.yaml (3)
2-2: LGTM!Version bump is consistent with the changelog entry.
40-40: LGTM!The default value "desired" is appropriate and aligns with the documentation.
64-64: LGTM!The schema correctly defines the three allowed encryption values.
| - "match(^(?i:(addons|addon_configs|backup|config|media|share|ssl))$)" | ||
| compatibility_mode: bool | ||
| apple_compatibility_mode: bool | ||
| encryption: list(off|desired|required) |
There was a problem hiding this comment.
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).
| - ssl | ||
| compatibility_mode: false | ||
| apple_compatibility_mode: true | ||
| encryption: "desired" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Do you know why the Samba developers did not choose this option by default?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If we provide
defaultas 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.
| name: Encryption | ||
| description: >- | ||
| Configure SMB encryption. | ||
| SMB3 protocol is used if set to required. |
There was a problem hiding this comment.
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.
| smb encrypt = {{ .encryption }} | ||
| {{ if eq .encryption "required" }} | ||
| client min protocol = SMB3 | ||
| client max protocol = SMB3 |
There was a problem hiding this comment.
Client features are not used by the add-on so this doesn't really do anything.
| interfaces = lo {{ .interfaces | join " " }} | ||
| hosts allow = 127.0.0.1 {{ .allow_hosts | join " " }} | ||
|
|
||
| smb encrypt = {{ .encryption }} |
There was a problem hiding this comment.
To align with the other config uses:
| smb encrypt = {{ .encryption }} | |
| server smb encrypt = {{ .encryption }} |
| {{ if eq .encryption "required" }} | ||
| client min protocol = SMB3 | ||
| client max protocol = SMB3 | ||
| server min protocol = SMB3 |
There was a problem hiding this comment.
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
|
|
||
| ## 12.5.5 | ||
|
|
||
| - Add option for setting smb encrypted parameter |
There was a problem hiding this comment.
This needs a note that the default changes.
| - ssl | ||
| compatibility_mode: false | ||
| apple_compatibility_mode: true | ||
| encryption: "desired" |
There was a problem hiding this comment.
If we provide
defaultas 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.
3 participants
Adds option to set "smb encrypt" to off/desired/required. If set to required, all traffic between client and server will be encrypted instead of plain text message traffic.
Encryption requires to use SMB3 and cannot be used in combination with compatibility mode option.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.