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

smbclient: cleanup smbclient configuration #12110

Merged
merged 1 commit into from Aug 3, 2017

Conversation

@chewitt
Copy link
Member

commented May 16, 2017

This PR does some overdue cleanup on the smbclient configuration used by Kodi.

Description

  • Removed by fritch: socket options tuning that is not recommended: https://lists.samba.org/archive/samba/2013-February/171836.html

  • Removes preferred master, local master, domain master smbd configuration that is irrelevant to smbclient.

  • Removes client lanman auth so smbclient can use more secure hash algorithms. This breaks compatibility with Windows 95/98 and pre Samba 2.2 shares (c.2000) that do not support NTLM or NTLMv2.

  • Removes lanman auth smbd configuration that is irrelevant to smbclient.

  • Sets client max protocol to SMB3. Without this smbclient (and thus Kodi) defaults to NT1 (SMB1) and will not attempt connecting at higher protocol versions. Setting to SMB3 allows smbclient to auto-negotiate a higher protocol with most devices. Some older Windows and Samba versions can require client max protocol to be reduced to SMB2 or SMB1 to solve connectivity issues (server side bugs or misconfiguration) so we expose this as a configuration option in Kodi settings. Using SMB3 with smbclient versions below 4.1 that do not support SMB2+ are not affected; older smbclient versions simply negotiate up to what they understand, i.e. NT1. This is part based on koying/SPMC@833aacd that forces NT1 connectivity.

  • Fixes a bug where smb.conf was not written if the file was not already present. The smb.conf file is also created under the Kodi home folder to prevent clashes with other smb.conf files that may exist on the users' system. The smb.con file is rewritten if the user changes the client max protocol setting (and the user is prompted to restart Kodi to effect the change). This borrows from koying/SPMC@2771cb2 that made rewrite on start user configurable and other ideas from Slack.

Motivation and Context

Wannacry ransomware attacks have woken people up to the security issues with SMB1 and Kodi refusing to use anything newer forces people to continue using insecure shares. LE plans to use a Krypton version of this with samba 4.6 in a future release to bring SMB2/3 support. NB: This PR is intended as a nip/tuck improvement to the existing regime until someone comes up with a more long-term replacement for smbclient.

How Has This Been Tested?

Lots of experimentation and detective work from @MilhouseVH. An earlier variant of the max client protocol change and samba 4 have been in his nightly builds for several months and this specific PR will be included shortly. It has not been tested with other Kodi build targets that use smbclient (macOS, Android, etc.) but testing with older smbclient 3.6 on Linux hasn't thrown up any obvious issues.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

https://lists.samba.org/archive/samba-technical/2016-November/117005.html

Samba defaults to NT1 client connections unless client max protocol is configured, which explains why Kodi is fixated on establishing NT1 connections regardless of what is (or is not) supported by the server.

I've just checked the latest Samba master[1] and the default client connection is still(!) NT1... utter madness.

  1. https://github.com/samba-team/samba/blob/7556c20d4bf90bfcc288ba1c82008105eaf8f261/source3/param/loadparm.c#L4529-L4532 <--- current HEAD on master
@@ -4065,7 +4065,9 @@ msgctxt "#1208"
msgid "Mount SMB shares"
msgstr ""

#empty string with id 1209
msgctxt "#1209"

This comment has been minimized.

Copy link
@ksooo

ksooo May 16, 2017

Member

Could you please provide contextual information for the new strings (here and elsewhere). This makes life of translators a lot easier. Thanks.
#. ...
#: ...

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser May 16, 2017

Member

Also do not use strings below 10000

@koying

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

You probably do not want to force rewrite smb.conf on a plain linux distro. Or do I miss something?
I mean the user might have a custom one, already

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch from 2f79a38 to 253152c May 16, 2017

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

Or do I miss something?

The problem is that currently the config isn't updated at all, even if a setting is changed. This currently requires the user to delete the .smb folder before new settings can be applied.

What is the problem with updating $HOME/.smb/smb.conf every time Kodi is started - are you concerned about other non-Kodi applications that might be sharing smb.conf?

@koying

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

Yes. smb.conf is the standard user libsmbclient config file, so other apps or even the user might have tweaked it on a Linux distro, and we probably should not unconditionally overwrite it.

But I'll completely trust @wsnipex 's opinion on this.

PS BTW, this is exactly why I have an "overwrite" setting in spmc although it's not needed for Android.
Not saying it's the best solution, either, just that it was the quickest ;)

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch 2 times, most recently from 2fdca9f to 6c98742 May 17, 2017

@koying

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

Note that an automagic way to overwrite config could be to calculate the hash of the existing file and compare with a precalculated one of our "old" default.
If equal, we can assume it's ours and overwrite.

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch from 6c98742 to c2f5da4 May 17, 2017

@wsnipex

This comment has been minimized.

Copy link
Member

commented May 17, 2017

We should never overwrite anything outside of our userdata dir.

why don't we use our own smb.conf instead of the standard one? That way there is no problem with overwriting.

And the client max protocol won't work anywhere but linux until we somehow manage to upgrade samba to a more recent version. So this setting should only be visible if supported.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 17, 2017

So this setting should only be visible if supported.

@wsnipex then this would need run-time detection of the smb library version. I can build Kodi on Ubuntu 16.04 which has support for SMB2 and SMB3 client connections (smb library 4.3.11). Also it doesn't appear easy to use an alternative location for smb.conf.

@koying an option to overwrite has been added to this PR overnight - once enabled it allows the WINS and max protocol settings to be changed (and written at next restart or when next new context is established). The overwrite setting will be automatically disabled once the new conf is written.

Not sure if a magic hash would work as once you change any of the defaults your precalculated "default" hash would no longer match and you'd never update the file again, not even to set it back to current defaults (plus this would make life harder for anyone using appliance.xml etc.). :)

@chewitt

This comment has been minimized.

Copy link
Member Author

commented May 17, 2017

@wsnipex If you run smbclient from the console you can append --config /path/to/smb.conf but there is no equivalent for API calls so we have to work with a potentially shared config.

@wsnipex

This comment has been minimized.

Copy link
Member

commented May 17, 2017

build time version check is enough. But @Rechi managed to get samba 4.1 built for osx, so there is hope :)

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch 2 times, most recently from 09e8e4d to 3edead1 May 17, 2017

fprintf(f, "\tclient lanman auth = yes\n");
fprintf(f, "\tlanman auth = yes\n");

fprintf(f, "\tsocket options = TCP_NODELAY IPTOS_LOWDELAY SO_RCVBUF=65536 SO_SNDBUF=65536\n");
fprintf(f, "\tlock directory = %s/.smb/\n", getenv("HOME"));

This comment has been minimized.

Copy link
@Rechi

Rechi May 23, 2017

Member

could you also replace getenv("HOME") with home.c_str() like you have done above?

@Rechi

This comment has been minimized.

Copy link
Member

commented May 23, 2017

@chewitt this needs to be rebased

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch from 3edead1 to 1f54b36 May 23, 2017

@chewitt

This comment has been minimized.

Copy link
Member Author

commented May 23, 2017

rebased .. do we want to add the home/env hack @koying suggested in Slack?

@Rechi

This comment has been minimized.

Copy link
Member

commented May 23, 2017

With this change smb.conf now gets overwrite although it exists already.
To avoid removing / changing some settings a user has set explicitly for the whole os, we should use our own smb.conf.

@koying

This comment has been minimized.

Copy link
Contributor

commented May 23, 2017

The only way I found to have our own smb.conf is:

    // HACK!! Force libsmbclient to use our own smb.conf by overriding HOME
    char* truehome = getenv("HOME");
    setenv("HOME", CSpecialProtocol::TranslatePath("special://home").c_str(), 1);

    // setup our context
    m_context = smbc_new_context();

    // HACK!! Restore HOME
    setenv("HOME", truehome, 1);

SMB_CONF_PATH didn't work for me

PS: Obviously, we have to create smb.conf in "special://home" as well.

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch 2 times, most recently from 98f4ae9 to 08836fa May 23, 2017

@koying

This comment has been minimized.

Copy link
Contributor

commented May 23, 2017

Note that, if we go the "own smb.conf" route, CSettings::SETTING_SMB_OVERWRITECONF doesn't make sense anymore

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented May 23, 2017

Note that, if we go the "own smb.conf" route, CSettings::SETTING_SMB_OVERWRITECONF doesn't make sense anymore

Yes, and no... being able to selectively overwrite would at least allow users to "tune" the smb.conf if they wished. And constant rewriting seems a little unnecessary.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jul 7, 2017

What's gonna happen with this and backport?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2017

It's been in my LE test builds for several weeks and is working as intended. It will produce an "invalid parameter" warning to stdout for those systems with libsmbclient prior to 4.1. Users with broken/ancient Samba servers that claim to support SMB3 (but don't) will need to configure SMB2 or SMB1 to restore Samba functionality.

Disabling NT1/SMB1 on the server (as recommended by everyone, which is the reason why this PR is becoming necessary) seems to break network browsing but that's an unrelated issue to this PR (and is a core Samba issue rather than Kodi).

@koying

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

Is it just me or is cache directory somewhat mandatory, now.
Without it, on my droid 4.5.1 build, I get mkdir failed on directory /home/cbro/spmc17-build/arm-linux-androideabi-21/var/cache/samba: No such file or directory, /home/cbro/spmc17-build/arm-linux-androideabi-21 being the default dir from build.

On 3.6, and in the doc, it's clearly stated that lock directory should be used, but it seems it's not...

@koying

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2017

I was looking at samba configuration, and stumbled across posts suggesting auto-negotiation (no "client max protocol" at all) is the preferred way.
So we might need a default "Default" option, which doesn't set the option at all...

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch from a2429aa to d00a376 Jul 20, 2017

@chewitt

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@koying PR is updated with a small tweak from @MilhouseVH so we default to no extra config and smbclient autonegotiates.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@chewitt the default is still SMBv3 which personally I'd keep, but to match the help message (which now says the default is auto-negotiate) this value needs to be changed to 0. The purpose of this PR however is to support better than NT1 and with more servers dropping NT1 (which will continue to be negotiated when the setting is auto, and thus not working) I think we should continue to use a default setting of SMBv3 - negotiating a lower protocol should become the exception and only when the user needs it.

@koying

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

Is "negotiation" not implying client and server agree on a mutually acceptable level?
I'd assume that if the server does not support a level, that one wouldn't be selected, or I don't see the point ;)
Anyway, I'll try to do tests with various combination this WE and report

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

Negotiation will only happen if client max protocol is greater than NT1 (aka SMBv1), ie. if SMBv2 or SMBv3 is selected.

Without client max protocol, libsmbclient will only ever use NT1 regardless of what the server may support (which may not include NT1 meaning the client is unable to establish a connection with the server).

Forcing NT1 should produce the same behaviour as not setting client max protocol, but apparently doesn't always, hence the request for a default/none/auto option (which I added in a patch to @chewitt and which didn't change the SMBv3 default - it just added a "do nothing" option).

In order for Samba to automatically negotiate a better than NT1 connection the client max protocol should be set to SMBv3 as the default, and then knocked back (or disabled) by the minority of users with duff servers.

@koying

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

Ok, thanks for the clarification.
Thinking out loud, but then shouldn't "Default" (=SMB3) + "SMB1" options be sufficient?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

We thought so, but then you asked for the "do nothing" option!

I guess it's possible that samba.org may flip the default connecting behaviour from "forced NT1" to "auto-negotiate highest version supported by this client" but until that happens then we have to assume NT1 will always be negotiated in the absence of a client max protocol.

Perhaps the posts you found that suggested not setting anything were either misinformed (you'd expect Samba to auto-negotiate, but that's not what it does by default) or were written from the perspective of ensuring maximum compatibility in an age when every server supported NT1 (not the case today).

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

And apparently it will be auto-negotiate by default with Samba 4.7

https://git.samba.org/?p=samba.git;a=commitdiff;h=1199907cbe2f003a7df6f56e6cf3878d0732344d

@koying

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

We thought so, but then you asked for the "do nothing" option!

Sorry about that ;)
Doing an actual google returns conflicting information on whether not specifying the option default to "NT1" or do auto-negotiation, probably dependent on the samba version.

it was just a suggestion btw, although reading me back might seem more imperative than I intended.

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch 2 times, most recently from 8eb915a to 7d550b9 Jul 21, 2017

// force libsmbclient to use our own smb.conf by overriding HOME
std::string truehome(getenv("HOME"));
setenv("HOME", CSpecialProtocol::TranslatePath("special://home").c_str(), 1);

// Create ~/.smb/smb.conf. This file is used by libsmbclient.

This comment has been minimized.

Copy link
@wsnipex

wsnipex Jul 21, 2017

Member

path is now ~/.kodi/.smb/smb.conf, correct?

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch 2 times, most recently from d1394e5 to 38c2e53 Jul 21, 2017

@koying

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

I confirm "auto-negotiation" is kind of BS ;)
From droid 4.5.1 to debian 4.2.14, if I disable smb1 on the NAS (with min protocol = SMB2) and leave out max protocol on the client, I just can't connect.

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch from 38c2e53 to 05fc9e0 Jul 22, 2017

@chewitt

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2017

The default option and wording is now set to "SMB3" to provide compatibility. Samba <4.1 will still connect using NT1, while Samba 4.1+ will connect using SMB3_11 and should step down to SMB2 if required. The none option (remove config and use Samba defaults) is retained as it does no harm and may be useful with support. The code comment on ~/.kodi/.smb/smb.conf path has been amended.

@koying
koying approved these changes Jul 22, 2017
Copy link
Contributor

left a comment

Keeping "None" as an option for the future makes sense.
+1, nice work :)

@wsnipex
Copy link
Member

left a comment

looks good now, thanks 👍

@chewitt chewitt force-pushed the chewitt:smbclient_leia branch from 05fc9e0 to 2b1402b Jul 22, 2017

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Aug 3, 2017

@MartijnKaijser MartijnKaijser merged commit 2c41c63 into xbmc:master Aug 3, 2017

1 check passed

default You're awesome. Have a cookie
Details
@thezoggy thezoggy referenced this pull request Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.