Re: [Qemu-devel] Bug#727756: qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

Re: [Qemu-devel] Bug#727756: qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Michael Tokarev]

26.10.2013 13:07, Michael Büsch wrote:
> Package: qemu
> Version: 1.6.0+dfsg-2
> Severity: normal
> Tags: patch
>
> The smb.conf automatically generated by qemu's -smb option fails on current samba,
> because smbd rejects the security=share option with the following warning:
>
>>    WARNING: Ignoring invalid value 'share' for parameter 'security'
>
> Which makes it fall back to security=user without guest login.
> This results in being unable to login to the samba server from the guest OS.
>
> The attached patch fixes this by selecting 'user' explicitly and mapping
> unknown users to guest logins.


Index: qemu-1.6.0+dfsg/net/slirp.c
===================================================================
--- qemu-1.6.0+dfsg.orig/net/slirp.c
+++ qemu-1.6.0+dfsg/net/slirp.c
@@ -529,7 +529,8 @@ static int slirp_smb(SlirpState* s, cons
              "state directory=%s\n"
              "log file=%s/log.smbd\n"
              "smb passwd file=%s/smbpasswd\n"
-            "security = share\n"
+            "security = user\n"
+            "map to guest = Bad User\n"
              "[qemu]\n"
              "path=%s\n"
              "read only=no\n"

Thank you for the report and the patch Michael.  Are you sure the result
is equivalent?  I mean, it *looks* like okay, because [qemu] share definition -
which looks like this:

             [qemu]
             path=%s
             read only=no
             guest ok=yes
             force user=%s

explicitly says that guest is okay, and forces user to the
right one.  And it should work the same with other versions
of samba too.

But samba collected so many semi-conflicting options with
years, so I'm not sure anymore.

Also, which users are "bad" -- will it be possible for our
user to "clash" with some built-in/known user?

Cc'ing qemu-devel@ because this needs to be resolved
upstream too.

Thank you again,

/mjt

Re: [Qemu-devel] Bug#727756: qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

On Sat, 26 Oct 2013 13:19:29 +0400
Michael Tokarev <mjt@tls.msk.ru> wrote:

> Thank you for the report and the patch Michael.  Are you sure the result
> is equivalent?

Well, I am far from being an SMB expert. So I can't really say whether this
is equivalent.
I also posted this patch to the qemu-devel list, but didn't get a reply, yet.

I tested this with a Windows XP client. Without this patch the
client will always ask for username and password. Which I am unable to
supply (smbpasswd is empty after all).
With this patch applied, the share works without authentication. And this
is how it used to work in previous versions, too.

> explicitly says that guest is okay, and forces user to the
> right one.  And it should work the same with other versions
> of samba too.

I only tried this with smbd from sid.
My guess is that it would work on older versions, too. But that is
untested.

> Also, which users are "bad" -- will it be possible for our
> user to "clash" with some built-in/known user?

'bad" users seem to be users that are not in the smbpasswd file.
As qemu creates an empty smbpasswd file, all users probably are "bad".
But I'm not sure if there are exceptions to that.

> Cc'ing qemu-devel@ because this needs to be resolved
> upstream too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] Bug#727756: qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Michael Tokarev]

26.10.2013 20:44, Michael Büsch wrote:
[]
>> Also, which users are "bad" -- will it be possible for our
>> user to "clash" with some built-in/known user?
>
> 'bad" users seem to be users that are not in the smbpasswd file.
> As qemu creates an empty smbpasswd file, all users probably are "bad".

That looks right.  Are you okay adding your Signed-off-by to the patch
you initially submitted?  If yes, I'll make a formal patch submission
upstream.

Because, well, this bothers several people already.

Thank you!

/mjt

Re: [Qemu-devel] Bug#727756: qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Michael Büsch]

[-- Attachment #1.1: Type: text/plain, Size: 255 bytes --]

On Fri, 01 Nov 2013 13:32:49 +0400
Michael Tokarev <mjt@tls.msk.ru> wrote:

> That looks right.  Are you okay adding your Signed-off-by to the patch
> you initially submitted?  If yes, I'll make a formal patch submission
> upstream.

Here you go.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: fix-smb-security-share.patch --]
[-- Type: text/x-patch, Size: 1199 bytes --]

From: Michael Buesch <m@bues.ch>
Subject: [PATCH] qemu/slirp: Fix SMB security configuration on newer samba versions

The smb.conf automatically generated by qemu's -smb option fails on current
samba, because smbd rejects the security=share option with the following warning:

>   WARNING: Ignoring invalid value 'share' for parameter 'security'  

Which makes it fall back to security=user without guest login.
This results in being unable to login to the samba server from the guest OS.

This fixes it by selecting 'user' explicitly and mapping
unknown users to guest logins.

Signed-off-by: Michael Buesch <m@bues.ch>

---

Index: qemu-1.6.0+dfsg/net/slirp.c
===================================================================
--- qemu-1.6.0+dfsg.orig/net/slirp.c
+++ qemu-1.6.0+dfsg/net/slirp.c
@@ -529,7 +529,8 @@ static int slirp_smb(SlirpState* s, cons
             "state directory=%s\n"
             "log file=%s/log.smbd\n"
             "smb passwd file=%s/smbpasswd\n"
-            "security = share\n"
+            "security = user\n"
+            "map to guest = Bad User\n"
             "[qemu]\n"
             "path=%s\n"
             "read only=no\n"

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [PATCH] qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Michael Tokarev]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

01.11.2013 13:54, Michael Büsch wrote:
> On Fri, 01 Nov 2013 13:32:49 +0400
> Michael Tokarev <mjt@tls.msk.ru> wrote:
>
>> That looks right.  Are you okay adding your Signed-off-by to the patch
>> you initially submitted?  If yes, I'll make a formal patch submission
>> upstream.
>
> Here you go.

Thank you!

Adding Jan as slirp maintainer, and my

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>

If Jan picks it up, that's fine.  If not, I think it can go
to the trivial patches queue.

Jan, if you missed context, it is in http://bugs.debian.org/727756
It is not the first time this issue pops up on qemu-devel@

/mjt


[-- Attachment #2: fix-smb-security-share.patch --]
[-- Type: text/x-patch, Size: 1167 bytes --]

From: Michael Buesch <m@bues.ch>
Subject: [PATCH] qemu/slirp: Fix SMB security configuration on newer samba versions

The smb.conf automatically generated by qemu's -smb option fails on current
samba, because smbd rejects the security=share option with the following warning:

>   WARNING: Ignoring invalid value 'share' for parameter 'security'  

Which makes it fall back to security=user without guest login.
This results in being unable to login to the samba server from the guest OS.

This fixes it by selecting 'user' explicitly and mapping
unknown users to guest logins.

Signed-off-by: Michael Buesch <m@bues.ch>

---

Index: qemu-1.6.0+dfsg/net/slirp.c
===================================================================
--- qemu-1.6.0+dfsg.orig/net/slirp.c
+++ qemu-1.6.0+dfsg/net/slirp.c
@@ -529,7 +529,8 @@ static int slirp_smb(SlirpState* s, cons
             "state directory=%s\n"
             "log file=%s/log.smbd\n"
             "smb passwd file=%s/smbpasswd\n"
-            "security = share\n"
+            "security = user\n"
+            "map to guest = Bad User\n"
             "[qemu]\n"
             "path=%s\n"
             "read only=no\n"

Re: [Qemu-devel] [PATCH] qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

On 2013-11-01 11:10, Michael Tokarev wrote:
> 01.11.2013 13:54, Michael Büsch wrote:
>> On Fri, 01 Nov 2013 13:32:49 +0400
>> Michael Tokarev <mjt@tls.msk.ru> wrote:
>>
>>> That looks right.  Are you okay adding your Signed-off-by to the patch
>>> you initially submitted?  If yes, I'll make a formal patch submission
>>> upstream.
>>
>> Here you go.
> 
> Thank you!
> 
> Adding Jan as slirp maintainer, and my
> 
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> 
> If Jan picks it up, that's fine.  If not, I think it can go
> to the trivial patches queue.

Works fine, applied to queues/slirp.

But this is not a trivial patch as the fix is not obvious for a reader
(unless you know smb.conf semantics by heart).

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Michael Tokarev]

04.11.2013 00:06, Jan Kiszka wrote:
> On 2013-11-01 11:10, Michael Tokarev wrote:
[]
>> If Jan picks it up, that's fine.  If not, I think it can go
>> to the trivial patches queue.
>
> Works fine, applied to queues/slirp.

Okay, thank you Jan.

> But this is not a trivial patch as the fix is not obvious for a reader
> (unless you know smb.conf semantics by heart).

It's trivial for my understanding.  If we require that every
change going to -trivial should be obvious to everyone, we
should just close it right away.

And this area does not have an active maintainer anyway, at least
according to MAINTAINERS and ./scripts/get_maintainer.pl.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] qemu: Broken -smb with latest SAMBA package. (Unsupported security=share option)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On 2013-11-04 14:55, Michael Tokarev wrote:
> 04.11.2013 00:06, Jan Kiszka wrote:
>> On 2013-11-01 11:10, Michael Tokarev wrote:
> []
>>> If Jan picks it up, that's fine.  If not, I think it can go
>>> to the trivial patches queue.
>>
>> Works fine, applied to queues/slirp.
> 
> Okay, thank you Jan.
> 
>> But this is not a trivial patch as the fix is not obvious for a reader
>> (unless you know smb.conf semantics by heart).
> 
> It's trivial for my understanding.  If we require that every
> change going to -trivial should be obvious to everyone, we
> should just close it right away.

Then we may need -less-trivial, because - to my understanding - -trivial
was once set up according to the rule that (most) QEMU hackers should be
able to understand that a trivial change is at least mostly harmless.

> And this area does not have an active maintainer anyway, at least
> according to MAINTAINERS and ./scripts/get_maintainer.pl.

Yeah, I think we have some holes there. That smb configuration
conceptually belongs to slirp is right, just not clear documented in our
script.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]