All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cifs: fix set of group SID via NTSD xattrs
@ 2022-01-03 14:50 Amir Goldstein
  2022-01-03 16:56 ` Protopopov, Boris
  2022-02-12  7:52 ` Steve French
  0 siblings, 2 replies; 5+ messages in thread
From: Amir Goldstein @ 2022-01-03 14:50 UTC (permalink / raw)
  To: Steve French, Boris Protopopov
  Cc: Pavel Shilovsky, Shyam Prasad N, linux-cifs

'setcifsacl -g <SID>' silently fails to set the group SID on server.

Actually, the bug existed since commit 438471b67963 ("CIFS: Add support
for setting owner info, dos attributes, and create time"), but this fix
will not apply cleanly to kernel versions <= v5.10.

Fixes: a9352ee926eb ("SMB3: Add support for getting and setting SACLs")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Boris,

I am a little confused from the comments in the code an emails.
In this thread [1] you wrote that you tested "setting/getting the owner,
DACL, and SACL...".

Does it mean that you did not test setting group SID?

It is also confusing that comments in the code says /* owner plus DACL */
and /* owner/DACL/SACL */.
Does it mean that setting group SID is not supposed to be supported?
Or was this just an oversight?

Anyway, with this patch, setcifsacl -g <SID> works as expected,
at least when the server is samba.

Thanks,
Amir.


[1] https://lore.kernel.org/linux-cifs/CAHhKpQ7PwgDysS3nUAA0ALLdMZqnzG6q6wL1tmn3aqOPwZbyyg@mail.gmail.com/

 fs/cifs/xattr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index 7d8b72d67c80..9d486fbbfbbd 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -175,11 +175,13 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
 				switch (handler->flags) {
 				case XATTR_CIFS_NTSD_FULL:
 					aclflags = (CIFS_ACL_OWNER |
+						    CIFS_ACL_GROUP |
 						    CIFS_ACL_DACL |
 						    CIFS_ACL_SACL);
 					break;
 				case XATTR_CIFS_NTSD:
 					aclflags = (CIFS_ACL_OWNER |
+						    CIFS_ACL_GROUP |
 						    CIFS_ACL_DACL);
 					break;
 				case XATTR_CIFS_ACL:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix set of group SID via NTSD xattrs
  2022-01-03 14:50 [PATCH] cifs: fix set of group SID via NTSD xattrs Amir Goldstein
@ 2022-01-03 16:56 ` Protopopov, Boris
  2022-01-03 18:25   ` Amir Goldstein
  2022-02-12  7:52 ` Steve French
  1 sibling, 1 reply; 5+ messages in thread
From: Protopopov, Boris @ 2022-01-03 16:56 UTC (permalink / raw)
  To: Amir Goldstein, Steve French; +Cc: Pavel Shilovsky, Shyam Prasad N, linux-cifs

Hello, Amir, 

It has been a while, but I recall that from my reading of the MS docs, the notion of "owner" was supposed to include both user and the primary group SIDs, which is why the comments in the code did not call out groups explicitly.
I also recall that during development, I tested with CIFS_ACL_GROUP flag against Windows 2012 and 2019 servers, and did not see a difference. I did not test against Samba, which clearly, showed an issue discussed below.

Boris.

On 1/3/22, 9:51 AM, "Amir Goldstein" <amir73il@gmail.com> wrote:

    CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



    'setcifsacl -g <SID>' silently fails to set the group SID on server.

    Actually, the bug existed since commit 438471b67963 ("CIFS: Add support
    for setting owner info, dos attributes, and create time"), but this fix
    will not apply cleanly to kernel versions <= v5.10.

    Fixes: a9352ee926eb ("SMB3: Add support for getting and setting SACLs")
    Signed-off-by: Amir Goldstein <amir73il@gmail.com>
    ---

    Boris,

    I am a little confused from the comments in the code an emails.
    In this thread [1] you wrote that you tested "setting/getting the owner,
    DACL, and SACL...".

    Does it mean that you did not test setting group SID?

    It is also confusing that comments in the code says /* owner plus DACL */
    and /* owner/DACL/SACL */.
    Does it mean that setting group SID is not supposed to be supported?
    Or was this just an oversight?

    Anyway, with this patch, setcifsacl -g <SID> works as expected,
    at least when the server is samba.

    Thanks,
    Amir.


    [1] https://lore.kernel.org/linux-cifs/CAHhKpQ7PwgDysS3nUAA0ALLdMZqnzG6q6wL1tmn3aqOPwZbyyg@mail.gmail.com/

     fs/cifs/xattr.c | 2 ++
     1 file changed, 2 insertions(+)

    diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
    index 7d8b72d67c80..9d486fbbfbbd 100644
    --- a/fs/cifs/xattr.c
    +++ b/fs/cifs/xattr.c
    @@ -175,11 +175,13 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
                                    switch (handler->flags) {
                                    case XATTR_CIFS_NTSD_FULL:
                                            aclflags = (CIFS_ACL_OWNER |
    +                                                   CIFS_ACL_GROUP |
                                                        CIFS_ACL_DACL |
                                                        CIFS_ACL_SACL);
                                            break;
                                    case XATTR_CIFS_NTSD:
                                            aclflags = (CIFS_ACL_OWNER |
    +                                                   CIFS_ACL_GROUP |
                                                        CIFS_ACL_DACL);
                                            break;
                                    case XATTR_CIFS_ACL:
    --
    2.25.1



^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix set of group SID via NTSD xattrs
  2022-01-03 16:56 ` Protopopov, Boris
@ 2022-01-03 18:25   ` Amir Goldstein
  2022-01-03 18:31     ` Protopopov, Boris
  0 siblings, 1 reply; 5+ messages in thread
From: Amir Goldstein @ 2022-01-03 18:25 UTC (permalink / raw)
  To: Protopopov, Boris
  Cc: Steve French, Pavel Shilovsky, Shyam Prasad N, linux-cifs,
	samba-technical

On Mon, Jan 3, 2022 at 6:56 PM Protopopov, Boris <pboris@amazon.com> wrote:
>
> Hello, Amir,
>
> It has been a while, but I recall that from my reading of the MS docs, the notion of "owner" was supposed to include both user and the primary group SIDs, which is why the comments in the code did not call out groups explicitly.
> I also recall that during development, I tested with CIFS_ACL_GROUP flag against Windows 2012 and 2019 servers, and did not see a difference. I did not test against Samba, which clearly, showed an issue discussed below.

Interesting.
I admit that the language of the docs is ambiguous:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/ee9614c4-be54-4a3c-98f1-769a7032a0e4
"...flags indicating what security attributes MUST be applied".
So attributes whose flag is not set (e.g. group SID) MAY be applied or
MUST NOT be applied?
Perhaps samba would want to be as relaxed as Windows about the ACL_GROUP flag.

In any case, I don't see a reason not to set the flag when the group
SID information
is present.

Thanks,
Amir.

>
> Boris.
>
> On 1/3/22, 9:51 AM, "Amir Goldstein" <amir73il@gmail.com> wrote:
>
>     CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
>     'setcifsacl -g <SID>' silently fails to set the group SID on server.
>
>     Actually, the bug existed since commit 438471b67963 ("CIFS: Add support
>     for setting owner info, dos attributes, and create time"), but this fix
>     will not apply cleanly to kernel versions <= v5.10.
>
>     Fixes: a9352ee926eb ("SMB3: Add support for getting and setting SACLs")
>     Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>     ---
>
>     Boris,
>
>     I am a little confused from the comments in the code an emails.
>     In this thread [1] you wrote that you tested "setting/getting the owner,
>     DACL, and SACL...".
>
>     Does it mean that you did not test setting group SID?
>
>     It is also confusing that comments in the code says /* owner plus DACL */
>     and /* owner/DACL/SACL */.
>     Does it mean that setting group SID is not supposed to be supported?
>     Or was this just an oversight?
>
>     Anyway, with this patch, setcifsacl -g <SID> works as expected,
>     at least when the server is samba.
>
>     Thanks,
>     Amir.
>
>
>     [1] https://lore.kernel.org/linux-cifs/CAHhKpQ7PwgDysS3nUAA0ALLdMZqnzG6q6wL1tmn3aqOPwZbyyg@mail.gmail.com/
>
>      fs/cifs/xattr.c | 2 ++
>      1 file changed, 2 insertions(+)
>
>     diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
>     index 7d8b72d67c80..9d486fbbfbbd 100644
>     --- a/fs/cifs/xattr.c
>     +++ b/fs/cifs/xattr.c
>     @@ -175,11 +175,13 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
>                                     switch (handler->flags) {
>                                     case XATTR_CIFS_NTSD_FULL:
>                                             aclflags = (CIFS_ACL_OWNER |
>     +                                                   CIFS_ACL_GROUP |
>                                                         CIFS_ACL_DACL |
>                                                         CIFS_ACL_SACL);
>                                             break;
>                                     case XATTR_CIFS_NTSD:
>                                             aclflags = (CIFS_ACL_OWNER |
>     +                                                   CIFS_ACL_GROUP |
>                                                         CIFS_ACL_DACL);
>                                             break;
>                                     case XATTR_CIFS_ACL:
>     --
>     2.25.1
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix set of group SID via NTSD xattrs
  2022-01-03 18:25   ` Amir Goldstein
@ 2022-01-03 18:31     ` Protopopov, Boris
  0 siblings, 0 replies; 5+ messages in thread
From: Protopopov, Boris @ 2022-01-03 18:31 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Steve French, Pavel Shilovsky, Shyam Prasad N, linux-cifs,
	samba-technical

Hi, Amir,
I agree the language is ambiguous. I also think that including the flag should not be harmful in any way (I do not recall observing any unwanted side effects).
Thanks for addressing the issue found in testing with Samba!
Boris. 

On 1/3/22, 1:26 PM, "Amir Goldstein" <amir73il@gmail.com> wrote:

    CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



    On Mon, Jan 3, 2022 at 6:56 PM Protopopov, Boris <pboris@amazon.com> wrote:
    >
    > Hello, Amir,
    >
    > It has been a while, but I recall that from my reading of the MS docs, the notion of "owner" was supposed to include both user and the primary group SIDs, which is why the comments in the code did not call out groups explicitly.
    > I also recall that during development, I tested with CIFS_ACL_GROUP flag against Windows 2012 and 2019 servers, and did not see a difference. I did not test against Samba, which clearly, showed an issue discussed below.

    Interesting.
    I admit that the language of the docs is ambiguous:
    https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-smb2/ee9614c4-be54-4a3c-98f1-769a7032a0e4
    "...flags indicating what security attributes MUST be applied".
    So attributes whose flag is not set (e.g. group SID) MAY be applied or
    MUST NOT be applied?
    Perhaps samba would want to be as relaxed as Windows about the ACL_GROUP flag.

    In any case, I don't see a reason not to set the flag when the group
    SID information
    is present.

    Thanks,
    Amir.

    >
    > Boris.
    >
    > On 1/3/22, 9:51 AM, "Amir Goldstein" <amir73il@gmail.com> wrote:
    >
    >     CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
    >
    >
    >
    >     'setcifsacl -g <SID>' silently fails to set the group SID on server.
    >
    >     Actually, the bug existed since commit 438471b67963 ("CIFS: Add support
    >     for setting owner info, dos attributes, and create time"), but this fix
    >     will not apply cleanly to kernel versions <= v5.10.
    >
    >     Fixes: a9352ee926eb ("SMB3: Add support for getting and setting SACLs")
    >     Signed-off-by: Amir Goldstein <amir73il@gmail.com>
    >     ---
    >
    >     Boris,
    >
    >     I am a little confused from the comments in the code an emails.
    >     In this thread [1] you wrote that you tested "setting/getting the owner,
    >     DACL, and SACL...".
    >
    >     Does it mean that you did not test setting group SID?
    >
    >     It is also confusing that comments in the code says /* owner plus DACL */
    >     and /* owner/DACL/SACL */.
    >     Does it mean that setting group SID is not supposed to be supported?
    >     Or was this just an oversight?
    >
    >     Anyway, with this patch, setcifsacl -g <SID> works as expected,
    >     at least when the server is samba.
    >
    >     Thanks,
    >     Amir.
    >
    >
    >     [1] https://lore.kernel.org/linux-cifs/CAHhKpQ7PwgDysS3nUAA0ALLdMZqnzG6q6wL1tmn3aqOPwZbyyg@mail.gmail.com/
    >
    >      fs/cifs/xattr.c | 2 ++
    >      1 file changed, 2 insertions(+)
    >
    >     diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
    >     index 7d8b72d67c80..9d486fbbfbbd 100644
    >     --- a/fs/cifs/xattr.c
    >     +++ b/fs/cifs/xattr.c
    >     @@ -175,11 +175,13 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
    >                                     switch (handler->flags) {
    >                                     case XATTR_CIFS_NTSD_FULL:
    >                                             aclflags = (CIFS_ACL_OWNER |
    >     +                                                   CIFS_ACL_GROUP |
    >                                                         CIFS_ACL_DACL |
    >                                                         CIFS_ACL_SACL);
    >                                             break;
    >                                     case XATTR_CIFS_NTSD:
    >                                             aclflags = (CIFS_ACL_OWNER |
    >     +                                                   CIFS_ACL_GROUP |
    >                                                         CIFS_ACL_DACL);
    >                                             break;
    >                                     case XATTR_CIFS_ACL:
    >     --
    >     2.25.1
    >
    >


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix set of group SID via NTSD xattrs
  2022-01-03 14:50 [PATCH] cifs: fix set of group SID via NTSD xattrs Amir Goldstein
  2022-01-03 16:56 ` Protopopov, Boris
@ 2022-02-12  7:52 ` Steve French
  1 sibling, 0 replies; 5+ messages in thread
From: Steve French @ 2022-02-12  7:52 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Boris Protopopov, Pavel Shilovsky, Shyam Prasad N, CIFS, ronnie sahlberg

tentatively merged into cifs-2.6.git for-next pending testing

If you have ideas on how to add a test case for this, that would be
helpful (we have some cifs.ko specific tests we run in addition to the
usual xfstests and git filesystem regression tests).

On Mon, Jan 3, 2022 at 8:50 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> 'setcifsacl -g <SID>' silently fails to set the group SID on server.
>
> Actually, the bug existed since commit 438471b67963 ("CIFS: Add support
> for setting owner info, dos attributes, and create time"), but this fix
> will not apply cleanly to kernel versions <= v5.10.
>
> Fixes: a9352ee926eb ("SMB3: Add support for getting and setting SACLs")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Boris,
>
> I am a little confused from the comments in the code an emails.
> In this thread [1] you wrote that you tested "setting/getting the owner,
> DACL, and SACL...".
>
> Does it mean that you did not test setting group SID?
>
> It is also confusing that comments in the code says /* owner plus DACL */
> and /* owner/DACL/SACL */.
> Does it mean that setting group SID is not supposed to be supported?
> Or was this just an oversight?
>
> Anyway, with this patch, setcifsacl -g <SID> works as expected,
> at least when the server is samba.
>
> Thanks,
> Amir.
>
>
> [1] https://lore.kernel.org/linux-cifs/CAHhKpQ7PwgDysS3nUAA0ALLdMZqnzG6q6wL1tmn3aqOPwZbyyg@mail.gmail.com/
>
>  fs/cifs/xattr.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index 7d8b72d67c80..9d486fbbfbbd 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -175,11 +175,13 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
>                                 switch (handler->flags) {
>                                 case XATTR_CIFS_NTSD_FULL:
>                                         aclflags = (CIFS_ACL_OWNER |
> +                                                   CIFS_ACL_GROUP |
>                                                     CIFS_ACL_DACL |
>                                                     CIFS_ACL_SACL);
>                                         break;
>                                 case XATTR_CIFS_NTSD:
>                                         aclflags = (CIFS_ACL_OWNER |
> +                                                   CIFS_ACL_GROUP |
>                                                     CIFS_ACL_DACL);
>                                         break;
>                                 case XATTR_CIFS_ACL:
> --
> 2.25.1
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-12  7:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-03 14:50 [PATCH] cifs: fix set of group SID via NTSD xattrs Amir Goldstein
2022-01-03 16:56 ` Protopopov, Boris
2022-01-03 18:25   ` Amir Goldstein
2022-01-03 18:31     ` Protopopov, Boris
2022-02-12  7:52 ` Steve French

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.