turnkeylinux
Turnkey: Domain Controller - Samba "Winbind Options" broken
If one changes the options on the left side of the uI, and save those options, it does not change the settings in smb.conf. It does, however, seem to insert most of the settings in the smb.conf file. However, if you change these settings to no, and then return to the same page, those settings are all reverted to yes.
In addition, the option "winbind trusted domains only" was depreciated in Samba 4.8 (it was replaced with "winbind scan trusted domains"). This causes the smb.conf file to give an error that it is ignoring the command.
Steps to Reproduce
- Install & Configure Domain Controller package.
- Navigate to "Server" > "Samba Windows File Sharing" in the Webmin UI.
- Select the "Winbind Options" menu.
- Change any setting on the left side of the settings page provided to no.
- Select save.
- From terminal, run 'samba-tool testparm' to see the error introduced to the smb.conf file.
- Return t o the Webmin UI, and select "Winbind Options" again
- All options will be reverted to "yes".
Source of change to winbind trusted domains only Option in Samba Samba 4.8 Features Added
Current winbind options and Valid options in Samba 4.17 These are the available winbind options (source: samba.org 4.17 smb.conf man page). Note that any option marked with a * = default):
- neutralize nt4 emulation = <boolean yes/no*>
- reject md5 clients = <boolean yes/no*>
- reject md5 servers = <boolean yes/no*>
- require strong key = <boolean yes/no*>
- winbindd socket directory = <string, '${prefix}/var/run/winbindd'*>
- winbind:ignore domains =
{list of domains} - winbind cache time = <int, 300*>
- winbind expand groups = <int, 0*>
- winbind enum groups = <boolean yes/no*>
- winbind max clients = <int, 200*>
- winbind normalize names = <boolean yes/no*>
- winbind offline logon = <boolean yes/no*>
- winbind nested groups = <boolean yes*/no>
- winbind nss info = <sfu20 | rfc2307>
- winbind separator = <single character, '' default> ---Note that '+' not recommended.
- winbind sealed pipes = <boolean yes*/no>
- winbind scan trusted domains = <boolean yes/no*>
- winbind rpc only = <boolean yes/no*>
- winbind request timeout = <int, 60*> seconds
- winbind reconnect delay = <int, 30*> seconds
Winbind / Kerberos direct settings (when winbind controls Kerberos)
- kerberos encryption types =
- { all | strong | legacy }
- include system krb5 conf = <boolean yes*/no>
- winbind refresh tickets = <boolean yes/no*>
- winbind use krb5 enterprise principals = <boolean yes*/no>
Thanks for the detailed bug report @james8675309.
Just to clarify:
If one changes the options on the left side of the uI, and save those options, it does not change the settings in smb.conf. [...] It does, however, seem to insert most of the settings in the smb.conf file.
So it adds the new settings to the smb.conf, rather than updating the existing ones?
However, if you change these settings to no, and then return to the same page, those settings are all reverted to yes.
Has Samba been restarted after changing those settings? I suspect so, but if not please try that and see if there is any difference (good, bad or otherwise). IIRC Webmin has a button for that, or via CLI I'm pretty sure that the service is samba-ad-dc.
TBH, I doubt it will change anything, but worth a try.
Also, I assume this is the latest TurnKey Domain Controller appliance version (v18.1)? If you're unsure, run:
turnkey-version
Regardless we'll have a look ASAP.
It does not create new settings, it just changes those settings to yes no matter what's selected. It ignores 3 of the 2 settings. Restarting doesn't fix it. Details for clarification below.
The settings not setting the correct values has existed since at least the last half of the 17.x series, has definitely been in 18.0 and 18.1. It may have been longer, but I still had 17.1 container available to start up on a test vm and it does at least most of the test items below. Using that vm, I wiped the image for it and I installed a fresh 18.1 on it. Too hard to track rogue settings on a 50 line smbc.conf with those long strings. After install, I removed all settings, only keeping settings need to survive a restart.
- Selected only workgroup and realm in smb.conf (Note: I had to add the server role string back in in a few steps to restart the services) + the base installed netlogon shares.
- Opened the WebUI Winbind Settings page. Found that the default with no-data load to the initial webui settings are Enable Winbind=Yes, Trust Domain Users=No, List Users=yes, List Groups = yes, Use default domain=No, Kerberos = realmID and seconds for cache =300.
- On the left side, I chose the opposite of each of those settings, then clicked save. (This popped me back out to the main screen so I didn't get a screenshot), but I set Yes to the 2nd and 5th option, No to 1, 3, 4.
- -Immediately loaded the smb.conf file in the UI. Of the 5 options to select in the UI, only winbind use default and winbind trusted domains populate in the fresh smb.conf file. That means that the Enable Winbind, and Disallow Listing of User and Groups are not working at all.
-I then restarted both Winbind and Samba (after re-adding the server role, like an idiot), then went back the the webui. -I then checked the smb file, it's the same as before shutdown + a server role. -Loading the winbind option list, I find that all radios are now back on "yes" for all options.
-In the winbind UI, I then attempted change all of the yes options shown above (my original post mentioned this) to no. -In addition, I changed the cache setting from from default 300 to 305, and set UID /GID ranges of 10000-20000 then saved and restarted services. -I did not include LDAP because I don't have it installed on this test rig and don't desire to setup an LDAP server for this test... I'd rather setup 50 Windows NT4 servers then upgrade them to Active Directory rather than setup one openLDAP to work with Samba... Sorry!
-This is what the smb.conf file looks like:
workgroup =<redacted> server role = active directory domain controller winbind use default domain = yes idmap uid = 10000-20000 winbind trusted domains only = yes winbind cache time = 305 idmap gid = 10000-20000 realm = <redacted>
- After the services restarted, I returned to the Options Page.
- All "No" options are now yes again.
- The idmap values for user and group saved (options are only inserted into smb.conf if there is not a null value in those fields [NOTE: I am happy to say it is a null value only, there is a check for invalid white noise entries on the UID / GID input slots, an error is thrown if a space is put in a range: "Failed to save Winbind options : Invalid UID range - must be like 20000-30000"].
- The cache time settings changes as well (only inserts the option if default 300 value is changed in the WebUI. If it's default it's not in the file smb.conf.)
- Could not test Kerberos option as that would cause the services not to restart.
This is the journalctl -xeu smbd.service showing the error as far as the smb.conf error. The service does start regardless. There were no errors out of winbindd and nmbd.
Funny thing is that the "winbind trusted domains only" depreciated back in 2018, so that part of the bug has existed since either TK 16 when they moved to 4.9.5, or TK15 if the first move to Samba 4 was 4.8+. I've had to erase that line out of my own conf file more times than I can count, but luckily it's just an info error, it doesn't make the smb.conf file invalid.
Side Questions/Comments: 1-Is there a roadmap planned to update Samba to 4.19 or 4.21? At least 4.19? They added support for WIndows 2016 AD Schema in 4.19, which might interest some people. 2- Are you plannign to stick with Heimdal Kerberos as compiled, or is the plan to move to MIT? 3-Is there a way to disable Kerberos MIT from installation via TK if Samba-Heimdal is installed? I would highly recommend that the Kerberos version that is installed is listed. MIT Kerberos has taken the lead it seems, I've had two borked situations over accidently installing a MIT Kerberos package on a container once--- I might as well as rm'd my root. Another time I installed the TK Kerberos Module--- the uninitiated might be tempted to add a package or another TK Module that's for MIT Kerberos, not realizing the difference. Regardless, just a recommendation! 4- THANK YOU WHOEVER FIXED DNS-1 Let's Encrypt in 18.1! I'd give an exuberant high five if in your presence.
CONCLUSION Winbind options that work:
- Kerberos Realm on Domain Server
- Seconds to Cache User Details for
- Range of UID's for Windows Users
- Range of GID's for Windows Groups
Winbind Options that Don't work at all:
- Toggle for Listing Users
- Toggle for Listing Groups
- Toggle that enables/disables Winbind completely.
Winbind Options that place setting = yes option into the smb.conf file:
- Trust domain server users? (winbind trusted domains only) : Always sets the setting to YES, regardless of how you have it set. Also, if you edit any other file in the settings page, that setting gets re-added to the file if it was deleted or changed to yes if you had it set to no manually.
- Always use default domain? (winbind use default domain) : **Note:**This is actually a bad setting to have stuck on if one uses both Windows and Linux in their network. I had wondered until I just did this investigation how that got turned on a while back--- it must have happened when I updated a UID range for idmap.--- Regardless, the use default domain will almost always eventually cause pain if one uses a mixed domain. The option confuses Linux winbind or sssd implementations--- it causes users to not get matched to groups properly, which can cause some serious troubleshooting pains! Though it would be nice if Samba ever fixed it, as it's nice to be able to log into ssh with just a username that shares credentials across all boxes. :D
Please let me know if you need anything else! Thank you for your time!
One more thing I thought of. I manually changed the settings in smb.conf, and then loaded the UI page. It then displayed the correct settings. However, if you then click save it reverts them to yes. So whatever function handles the write to the file is apparently messed up. (Also seems to change the order of the smb.conf file quite a bit too).
I was unsure what settings "Enable Winbind for Local Accounts" was supposed to point at? or if it's mean to disable winbindd through systemctl?
So the options are:
- Enable Winbind for local accounts? = {UNKNOWN}
- Trust domain server users? = {winbind trusted domains only} (doesn't exist) other setting, but not winbind related. = Communicate with Trusted Domains? = {allow trusted domains} new option? Periodically Scan for trusted domains? = {winbind scan trusted domains}
- Disallow listing of users? = {winbind enum users}
- Disallow listing of groups? = {winbind enum groups}
- Always use default domain? = {winbind use default domain}
Thanks @james8675309 - TBH that's an incredible bug report! 😁 You rock. 👍
I'll have to have a closer look ASAP, but it certainly does sound like a specific Webmin issue. Whether it's an actual code bug, or just some sort of config issue (or perhaps a bit of both) remains to be seen.
FYI Webmin is a third party project - we just package and pre-install it. It's built to run on all different Linux distros and releases/versions, so is somewhat generic. From your detailed report, it seems likely that it's a Webmin bug, but it's also possible that there is some config that we need to tweak (something that needs fine tuning for Debian). Or perhaps both?
The packages we provide are a bit behind upstream, so I'll rebuild them first - just in case it's a bug they've already fixed. If its still reproducible on the latest version, I'll report it upstream and have a poke around the Webmin code too and see if anything jumps out.
Unfortunately (for TurnKey) Webmin is written in Perl and none of us are super familiar with it. But hopefully it'll be straight forward to work out what is going on.
Thanks again.
(edit: fixed formatting)... Finally had a chance to check what the issue was.
After a little investigating, I found that conf_bind.cgi had a bit of an error. You could see that the lines between the nonworking and working versions were differently formatted.
The not-working items were missing getval parts of their formatting. There were something like 5 or 6 of these iterations and then other variables were pulled in that do work, such as the gid and uid ranges for idmap.
So, on load the smb.conf file was not being loaded for those values. Becuase it wasn't reading these items, the loader was initializing the settings to their default, then saving them to the default value since they were 'empty". Regardless, the two files are down at the bottom complete.
Here is an effective template to explain:
#Bad Code Example Template
print &ui_table_row($text{'bind_%s'},
&ui_textbox("%s", %s))
#Fixed Code Example Template
print &ui_table_row($text{'bind_%s'},
&ui_textbox("%s", &getval("%s"),20));
Actual Example:
#Bad Code
print &ui_table_row($text{'bind_local'},
&yesno_input("winbind enable local accounts", ("local"));
#Good Code:
print &ui_table_row($text{'bind_local'},
&yesno_input("winbind enable local accounts", &getval("local")),20);
As such, I updated each setting that was missing that, as well as Realm. So I just inserted the getval on each 'realm', 'users', 'groups', 'trust' 'local and 'defaultdomain'. I also changed 'the 'trust' setting to 'winbind scan trusted domain', which is the correct value for the current iteration of Samba on these images.
I can now save and edit my settings using the UI! Pretty darn easily. Perl's not so bad!!!
The one change to save_bind":
#BEFORE:
&setval("winbind trusted domains only", $in{'trust'} ? "yes" : "no");
#AFTER: (Updated 'trust' to the setting for Samba post 4.9+):
&setval("winbind scan trusted domains", $in{'trust'} ? "yes" : "no");
Here are the full files:
LOADER conf_bind.cgi
# conf_bind.cgi
# Display winbind-related options
require './samba-lib.pl';
# check acls
&error_setup("$text{'eacl_aviol'}ask_epass.cgi");
&error("$text{'eacl_np'} $text{'eacl_pcm'}") unless $access{'conf_bind'};
&ui_print_header(undef, $text{'bind_title'}, "");
&get_share("global");
print &ui_form_start("save_bind.cgi", "post");
print &ui_table_start($text{'bind_title'}, undef, 2);
print &ui_table_row($text{'bind_local'},
&yesno_input("winbind enable local accounts", &getval("local")),20);
print &ui_table_row($text{'bind_trust'},
&yesno_input("winbind scan trusted domains", &getval("trust")),20);
print &ui_table_row($text{'bind_users'},
&yesno_input("winbind enum users", &getval("users")),20);
print &ui_table_row($text{'bind_groups'},
&yesno_input("winbind enum groups", &getval("groups")),20);
print &ui_table_row($text{'bind_defaultdomain'},
&yesno_input("winbind use default domain", &getval("defaultdomain")),20);
print &ui_table_row($text{'bind_realm'},
&ui_textbox("realm", &getval("realm"), 20));
print &ui_table_row($text{'bind_cache'},
&ui_textbox("cache", &getval("winbind cache time"), 20));
print &ui_table_row($text{'bind_uid'},
&ui_textbox("uid", &getval("idmap uid"), 20));
print &ui_table_row($text{'bind_gid'},
&ui_textbox("gid", &getval("idmap gid"), 20));
$backend = &getval("idmap backend");
print &ui_table_row($text{'bind_backend'},
&ui_radio("backend_def", $backend ? 0 : 1,
[ [ 1, $text{'default'} ],
[ 0, &ui_textbox("backend", $backend, 50) ] ]));
print &ui_table_end();
print &ui_form_end([ [ undef, $text{'save'} ] ]);
&ui_print_footer("", $text{'index_sharelist'});
save_bind.cgi
#!/usr/bin/perl
# save_bind.cgi
# Save inputs from conf_bind.cgi
require './samba-lib.pl';
&ReadParse();
&lock_file($config{'smb_conf'});
$global = &get_share("global");
# check acls
&error_setup("$text{'eacl_aviol'}ask_epass.cgi");
&error("$text{'eacl_np'} $text{'eacl_pcm'}") unless $access{'conf_bind'};
&error_setup($text{'bind_err'});
&setval("winbind enable local accounts", $in{'local'} ? "yes" : "no");
&setval("winbind scan trusted domains", $in{'trust'} ? "yes" : "no");
&setval("winbind enum users", $in{'users'} ? "yes" : "no");
&setval("winbind enum groups", $in{'groups'} ? "yes" : "no");
&setval("winbind use default domain", $in{'defaultdomain'} ? "yes" : "no");
$in{'realm'} eq "" || $in{'realm'} =~ /^\S+$/ || &error($text{'bind_erealm'});
&setval("realm", $in{'realm'});
$in{'cache'} =~ /^\d+$/ || &error($text{'bind_ecache'});
&setval("winbind cache time", $in{'cache'});
$in{'uid'} eq "" || $in{'uid'} =~ /^\d+\-\d+$/ || &error($text{'bind_euid'});
&setval("idmap uid", $in{'uid'});
$in{'gid'} eq "" || $in{'gid'} =~ /^\d+\-\d+$/ || &error($text{'bind_egid'});
&setval("idmap gid", $in{'gid'});
&setval("idmap backend", $in{'backend_def'} ? "" : $in{'backend'});
if ($global) { &modify_share("global", "global"); }
else { &create_share("global"); }
&unlock_file($config{'smb_conf'});
&webmin_log("bind", undef, undef, \%in);
&redirect("");
One question: when are you intending on updating passed Samba 4.17 on the AD image? There's a lot of changes to active directory since then, especially in the schema... this version is limited to Windows 2008 Active Directory without major modifcations, 4.19+ handles through 2012 and 2.21 handles through 2016 I do believe. ( might be off one version on that, but I know the latest native 4.21 Samba does support updating the schema to Win Server 2016 ). Thanks!. I compiled 4.21 using as close to the settings I could pull using the -b(uild) flag on the smb binary, but some things are a little tough to match. Regardless, I have a working newer version, so was just curious overall. Thanks!
Great work @james8675309 - you are a legend!
IMO an issue should be lodged upstream and a pull request including the fix you've documented here.
Do you want to do that? Or do you want me to?
Regardless, I was planning to update our Webmin packages soon anyway (the ones we distribute are quite old now) - so I can patch the files at build time.
Re your question:
One question: when are you intending on updating passed Samba 4.17 on the AD image? There's a lot of changes to active directory since then, especially in the schema... this version is limited to Windows 2008 Active Directory without major modifcations, 4.19+ handles through 2012 and 2.21 handles through 2016 I do believe. ( might be off one version on that, but I know the latest native 4.21 Samba does support updating the schema to Win Server 2016 ). Thanks!. I compiled 4.21 using as close to the settings I could pull using the -b(uild) flag on the smb binary, but some things are a little tough to match. Regardless, I have a working newer version, so was just curious overall. Thanks!
In v18.x we've installed samba from Debian "main" repo. As per Debian policy, it will almost certainly stay "frozen" at v4.17 until Debian 13/Trixie is released as stable (currently testing). However, samba v4.21 is in the Debian bookworm-backports repo.
So you can get v4.21 much easier than compiling from source yourself. Seeing as you've already installed from source, these likely aren't serious considerations for you, but there are some consideration when using packages from backports:
- they do not have the same level of testing as packages in main, so risk of bugs/unexpected behavior is higher
- timely security and/or bugfix related updates are not guaranteed
- manual updates are required - i.e. security updates will not be auto-installed - as per turnkey default
- generally security and/or bugfix updates are via a new version (rather than the minimal patches applies to the "frozen" version in main). I.e. if a security issue is found in v4.21 and is fixed in backports, you may get v4.22 (or newer as relevant) - meaning behavior may also change
Regardless, assuming that you want to install from debian backports; I suggest removing your self compiled version first - or at least removing the executable(s) from your PATH.
To make your life a little easier, in v18.x appliances we include a backports apt list, but it's disabled by default. To enable backports remove the .disabled suffix from the included sources.list. I.e.:
mv /etc/apt/sources.list.d/debian-backports.list.disabled /etc/apt/sources.list.d/debian-backports.list
I also suggest pinning the samba related packages to backports. Then you won't need to specify installing the backported packages when updating samba to v4.21. Do that like this:
cat > /etc/apt/preferences.d/samba-backports.pref <<EOF
Package: *samba*
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: *smb*
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: winbind
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: libnss-winbind
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: libpam-winbind
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: libwbclient0
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: libldb1
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: libldb2
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: libtalloc2
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: libtevent0
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: python3-ldb
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: python3-tdb
Pin: release n=bookworm-backports
Pin-Priority: 500
Package: python3-talloc
Pin: release n=bookworm-backports
Pin-Priority: 500
EOF
Then upgrade samba:
apt update
apt install -y samba
Let me know how you go.
Ahh, I was not aware of the backports thing.
When I compiled 4.21, I did it on a new Debian installation and installed Webmin separately, and only use that DC for my Windows 11 gaming machine. More for testing purposes, my main network is still running off of the Turnkey 18.1 image. I wasn't sure if 4.21 would fully interoperate with Turnkey's custom config, so I was just not going to tempt fate on my logon servers. It's only for a homelab to make logging in easier across quite a few baremetal and VM OS's as seemless as possible, so it's not the end of the world if it breaks--- well, I might hear about it if my fileserver is down, but I digress.... it took a while to get setup as I like it. Haha. This definitely gives me new latitude to tinker! Thank you!
I was intending this weekend to potentially do a pull request on the Webmin repository, but I found that in samba-lib.pl references these options in the error handling for if the options don't exist in the file. I got hung up a bit on the version check part, as I'm not sure where it's pulling the version string from and which format it's using (it's currently only discerning between version 3 and 4, and I need to pull the substring out as well). Currently my fix introduces another bug if someone is installing Webmin over a Samba version prior to 4.8. It works for Turnkey's version as 4.17 is pre-installed, but for me to upstream it it'd theoretically hit anyone running the older Samba, thus why I haven't yet. To be fair, my fix actually fixes two preexisting bugs for post 4.8 Samba and only leaves one for someone running a legacy version, so it's in the win column in my opinion, but I try to be considerate. Once I read the documentation on how the perl code pulls the version string, I'll share that fix here as well and then potentially submit a PR on the webmin repo.
However, looking through that repo, the files in question haven't been edited in 13 years, and they have Pull Requests still open going back to 2017, so if you have their ear it might be beneficial for you to submit it over me, as it might be sitting in PR hell for a while. Especially since the winbind bug would be at least 8 years old (4.8.0 release 3/13/18) and the Perl logic would be about 13 years old ( conf_bind was last updated Apr 1, 2012....).
If you want I'll post the samba-libs.pl file here with the fix without the version check for patching. I've been running it since my last post and have tested it with both 4.17 and 4.21 on my own servers. When I figure out the samba subversion check I will post the full fix here as well, and try a PR unless you think you might get more attention posting it?
Also, if you keep that list for yourself for the pinned packages, add "libtdb1" to that list. Samba 4.21 will not install without it. and libnss-winbind and/or libpam-winbind if someone wants to integrate logins into either the pam or use sss/realmd.
Ahh, I was not aware of the backports thing.
When I compiled 4.21, I did it on a new Debian installation and installed Webmin separately, and only use that DC for my Windows 11 gaming machine. More for testing purposes, my main network is still running off of the Turnkey 18.1 image. I wasn't sure if 4.21 would fully interoperate with Turnkey's custom config, so I was just not going to tempt fate on my logon servers. It's only for a homelab to make logging in easier across quite a few baremetal and VM OS's as seemless as possible, so it's not the end of the world if it breaks--- well, I might hear about it if my fileserver is down, but I digress.... it took a while to get setup as I like it. Haha. This definitely gives me new latitude to tinker! Thank you!
I haven't tested our default Fileserver Samba conf with v4.21, but I'd be fairly supprised if anything broke. OOTB the default Fileserver config is intended to support a network without an AD domain (i.e. just a bunch of PCs sharing files via SMB). So it's essentially Samba 3 config (which Samba explicitly still support). The only recent config tweaks we've made are related to changes in newer Windows releases, so the config is still essentially "Samba 3" style config.
You likely know better than me (I assume you are a Windows user?!) but AFAIK recent Windows versions no longer support "workgroup" style connection. So there are probably improvements we could make to the default config. So please don't hesitate to share any suggestions you have. In fact they would be warmly welcomed. None of us run Windows (except in a VM when testing a new Samba version).
I was intending this weekend to potentially do a pull request on the Webmin repository, but I found that in samba-lib.pl references these options in the error handling for if the options don't exist in the file. I got hung up a bit on the version check part, as I'm not sure where it's pulling the version string from and which format it's using (it's currently only discerning between version 3 and 4, and I need to pull the substring out as well). Currently my fix introduces another bug if someone is installing Webmin over a Samba version prior to 4.8. It works for Turnkey's version as 4.17 is pre-installed, but for me to upstream it it'd theoretically hit anyone running the older Samba, thus why I haven't yet. To be fair, my fix actually fixes two preexisting bugs for post 4.8 Samba and only leaves one for someone running a legacy version, so it's in the win column in my opinion, but I try to be considerate. Once I read the documentation on how the perl code pulls the version string, I'll share that fix here as well and then potentially submit a PR on the webmin repo.
No problem. FWIW I added your suggestions with the intention of testing locally and opening a PR myself. But before I got very far, it occurred to me that it would be a bit rude to jump in like that without at least giving you the option...
FWIW here's the commit: https://github.com/JedMeister/webmin-src/commit/58e7708031a5e58b464287bc52ae593467565c34
Given your rationale on not doing it yet and the fact that I'm snowed under with other stuff ATM, I'll leave it with you for now. Otherwise, I'll have a closer look once I've updated our Webmin packages and done some testing myself (no shade on you - I'm impressed with the lengths you've gone to).
However, looking through that repo, the files in question haven't been edited in 13 years, and they have Pull Requests still open going back to 2017, so if you have their ear it might be beneficial for you to submit it over me, as it might be sitting in PR hell for a while. Especially since the winbind bug would be at least 8 years old (4.8.0 release 3/13/18) and the Perl logic would be about 13 years old ( conf_bind was last updated Apr 1, 2012....).
I've had nothing but good experiences with Webmin upstream, although I don't think I've ever provided a PR (just issues and discussions).
If you want I'll post the samba-libs.pl file here with the fix without the version check for patching. I've been running it since my last post and have tested it with both 4.17 and 4.21 on my own servers. When I figure out the samba subversion check I will post the full fix here as well, and try a PR unless you think you might get more attention posting it?
As per the link above, I have already added the changes you noted to a personal fork of upstream. Feel free to use them yourself if you'd like. Also FYI a diff is generally better than uploading a full file (i.e. what GH displays in a commit). It's easier to see exactly what has changed. A diff is pretty easy with git, otherwise ou can use the diff command. I.e.:
diff -u old_file new_file > patch.txt
Anyway, it sounds like you've done a solid job testing and considering all the implications. If you don't get to it within the next few weeks, then I'll take care of it. If you do open a PR I'm sure the devs will engage.
Also, if you keep that list for yourself for the pinned packages, add "libtdb1" to that list. Samba 4.21 will not install without it. and libnss-winbind and/or libpam-winbind if someone wants to integrate logins into either the pam or use sss/realmd.
Thanks for this heads up too. I've updated the previous post.
I'm all for efficiency, thank you for making the PR. I'm new to actually using git, I've really only lurked for years and do bug reports all the time for software rollouts where I work since they consider me a 'power user'. In that case I'm more used to JIRA tickets than anything.
I did some digging on the commands.... the winbind enable local accounts existsed only in version 3.0.7 through 3.0.20...
We can replace it with something like this that is still in use and not slated for removal?
winbind normalize names (G)
This parameter controls whether winbindd will replace whitespace in user and group names with an underscore (_) character. For example, whether the name "Space Kadet" should be replaced with the string "space_kadet". Frequently Unix shell scripts will have difficulty with usernames contains whitespace due to the default field separator in the shell. If your domain possesses names containing the underscore character, this option may cause problems unless the name aliasing feature is supported by your nss_info plugin.
This feature also enables the name aliasing API which can be used to make domain user and group names to a non-qualified version. Please refer to the manpage for the configured idmap and nss_info plugin for the specifics on how to configure name aliasing for a specific configuration. Name aliasing takes precedence (and is mutually exclusive) over the whitespace replacement mechanism discussed previously.
Default: winbind normalize names = no
Example: winbind normalize names = yes