[PATCH] cifs: send workstation name during ntlmssp session setup

[PATCH] cifs: send workstation name during ntlmssp session setup

[Shyam Prasad N]

Hi Steve,

Please review this patch, and let me know what you think.
Having this info in the workstation field of session setup helps
server debugging in two ways.
1. It helps identify the client by node name.
2. It helps get the kernel release running on the client side.

https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch

I ran some basic testing with the patch. Seems to serve the purpose.
Please let me know if I'm missing something.

-- 
Regards,
Shyam

Re: [PATCH] cifs: send workstation name during ntlmssp session setup

[Enzo Matsumiya]

Hi Shyam,

I have some observations/suggestions regarding your patch:

On 11/06, Shyam Prasad N wrote:
>Hi Steve,
>
>Please review this patch, and let me know what you think.
>Having this info in the workstation field of session setup helps
>server debugging in two ways.
>1. It helps identify the client by node name.
>2. It helps get the kernel release running on the client side.
>
>https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch

- AFAICS it doesn't consider runtime hostname changes. Is it important
   to keep track of it? Would changing it mid-auth steps break it
   somehow?

- I didn't understand the purpose of CIFS_DEFAULT_WORKSTATION_NAME. Why
   not simply use utsname()->nodename? Or even init_utsname()->nodename, which
   is supposed to be always valid.

- Ditto for CIFS_MAX_WORKSTATION_LEN. utsname()->nodename has at most 65
   bytes (__NEW_UTS_LEN + 1) anyway. Perhaps using MAXHOSTNAMELEN from
   <asm/param.h> would be a more generic approach.
   (btw this is because nodename is the unqualified hostname, sans-domain)

- Instead of setting workstation_name to "nodename:release", why not
   implement the VERSION structure (MS-NLMP 2.2.2.10)? Then use
   LINUX_VERSION_* from <linux/version.h> or parse utsname()->release.

>I ran some basic testing with the patch. Seems to serve the purpose.
>Please let me know if I'm missing something.

I hope I didn't miss anything.


Cheers,

Enzo

Re: [PATCH] cifs: send workstation name during ntlmssp session setup

[Shyam Prasad N]

Thanks for the reviews, Paulo and Enzo. Please read my replies below:

On Sat, Nov 6, 2021 at 7:10 AM Steve French <smfrench@gmail.com> wrote:
>
> Interesting suggestions, probably worth experimenting with but even the original patch could help a lot eg help server to understand type of client connecting to it when debugging
>
> On Fri, Nov 5, 2021, 21:38 Enzo Matsumiya <ematsumiya@suse.de> wrote:
>>
>> Hi Shyam,
>>
>> I have some observations/suggestions regarding your patch:
>>
>> On 11/06, Shyam Prasad N wrote:
>> >Hi Steve,
>> >
>> >Please review this patch, and let me know what you think.
>> >Having this info in the workstation field of session setup helps
>> >server debugging in two ways.
>> >1. It helps identify the client by node name.
>> >2. It helps get the kernel release running on the client side.
>> >
>> >https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch
>>
>> - AFAICS it doesn't consider runtime hostname changes. Is it important
>>    to keep track of it? Would changing it mid-auth steps break it
>>    somehow?
I think that's okay. AFAIK, that's only used for
debugging/troubleshooting purposes. So it doesn't need to be a 100%
accurate.

>>
>> - I didn't understand the purpose of CIFS_DEFAULT_WORKSTATION_NAME. Why
>>    not simply use utsname()->nodename? Or even init_utsname()->nodename, which
>>    is supposed to be always valid.
I initially did not have the utsname changes. That idea was an afterthought.
Sure. I'll update the patch to fix this.

>>
>> - Ditto for CIFS_MAX_WORKSTATION_LEN. utsname()->nodename has at most 65
>>    bytes (__NEW_UTS_LEN + 1) anyway. Perhaps using MAXHOSTNAMELEN from
>>    <asm/param.h> would be a more generic approach.
>>    (btw this is because nodename is the unqualified hostname, sans-domain)
Noted.

>>
>> - Instead of setting workstation_name to "nodename:release", why not
>>    implement the VERSION structure (MS-NLMP 2.2.2.10)? Then use
>>    LINUX_VERSION_* from <linux/version.h> or parse utsname()->release.
That's a good idea. Let me explore that too.

>>
>> >I ran some basic testing with the patch. Seems to serve the purpose.
>> >Please let me know if I'm missing something.
>>
>> I hope I didn't miss anything.
>>
>>
>> Cheers,
>>
>> Enzo



-- 
Regards,
Shyam

Re: [PATCH] cifs: send workstation name during ntlmssp session setup

[Shyam Prasad N]

On Sat, Nov 6, 2021 at 9:08 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Thanks for the reviews, Paulo and Enzo. Please read my replies below:
>
> On Sat, Nov 6, 2021 at 7:10 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Interesting suggestions, probably worth experimenting with but even the original patch could help a lot eg help server to understand type of client connecting to it when debugging
> >
> > On Fri, Nov 5, 2021, 21:38 Enzo Matsumiya <ematsumiya@suse.de> wrote:
> >>
> >> Hi Shyam,
> >>
> >> I have some observations/suggestions regarding your patch:
> >>
> >> On 11/06, Shyam Prasad N wrote:
> >> >Hi Steve,
> >> >
> >> >Please review this patch, and let me know what you think.
> >> >Having this info in the workstation field of session setup helps
> >> >server debugging in two ways.
> >> >1. It helps identify the client by node name.
> >> >2. It helps get the kernel release running on the client side.
> >> >
> >> >https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch
> >>
> >> - AFAICS it doesn't consider runtime hostname changes. Is it important
> >>    to keep track of it? Would changing it mid-auth steps break it
> >>    somehow?
> I think that's okay. AFAIK, that's only used for
> debugging/troubleshooting purposes. So it doesn't need to be a 100%
> accurate.
>
> >>
> >> - I didn't understand the purpose of CIFS_DEFAULT_WORKSTATION_NAME. Why
> >>    not simply use utsname()->nodename? Or even init_utsname()->nodename, which
> >>    is supposed to be always valid.
> I initially did not have the utsname changes. That idea was an afterthought.
> Sure. I'll update the patch to fix this.
>
> >>
> >> - Ditto for CIFS_MAX_WORKSTATION_LEN. utsname()->nodename has at most 65
> >>    bytes (__NEW_UTS_LEN + 1) anyway. Perhaps using MAXHOSTNAMELEN from
> >>    <asm/param.h> would be a more generic approach.
> >>    (btw this is because nodename is the unqualified hostname, sans-domain)
> Noted.
>
> >>
> >> - Instead of setting workstation_name to "nodename:release", why not
> >>    implement the VERSION structure (MS-NLMP 2.2.2.10)? Then use
> >>    LINUX_VERSION_* from <linux/version.h> or parse utsname()->release.
> That's a good idea. Let me explore that too.

Hi Enzo,
One "problem" with using just the version field is that it has a
predefined format.
This may not help us in decoding what distro and if it is a custom
kernel. utsname()->release gives all that.
So I'm planning to use workstation_name in the "hostname:release"
format. Let me know if you see some issues with it.

>
> >>
> >> >I ran some basic testing with the patch. Seems to serve the purpose.
> >> >Please let me know if I'm missing something.
> >>
> >> I hope I didn't miss anything.
> >>
> >>
> >> Cheers,
> >>
> >> Enzo
>
>
>
> --
> Regards,
> Shyam



-- 
Regards,
Shyam

Re: [PATCH] cifs: send workstation name during ntlmssp session setup

[Stefan Metzmacher]

Hi Shyam,

>>>> Please review this patch, and let me know what you think.
>>>> Having this info in the workstation field of session setup helps
>>>> server debugging in two ways.
>>>> 1. It helps identify the client by node name.
>>>> 2. It helps get the kernel release running on the client side.
>>>>
>>>> https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch
>>>
>>> - AFAICS it doesn't consider runtime hostname changes. Is it important
>>>    to keep track of it? Would changing it mid-auth steps break it
>>>    somehow?
> I think that's okay. AFAIK, that's only used for
> debugging/troubleshooting purposes. So it doesn't need to be a 100%
> accurate.

That's not true, the workstation name is used for access checks.

[MS-ADA3] 2.353 Attribute userWorkstations
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-ada3/ec941bac-bc77-48f3-a1cf-79d773a91b6b

>>>
>>> - I didn't understand the purpose of CIFS_DEFAULT_WORKSTATION_NAME. Why
>>>    not simply use utsname()->nodename? Or even init_utsname()->nodename, which
>>>    is supposed to be always valid.
> I initially did not have the utsname changes. That idea was an afterthought.
> Sure. I'll update the patch to fix this.
> 
>>>
>>> - Ditto for CIFS_MAX_WORKSTATION_LEN. utsname()->nodename has at most 65
>>>    bytes (__NEW_UTS_LEN + 1) anyway. Perhaps using MAXHOSTNAMELEN from
>>>    <asm/param.h> would be a more generic approach.
>>>    (btw this is because nodename is the unqualified hostname, sans-domain)
> Noted.
> 
>>>
>>> - Instead of setting workstation_name to "nodename:release", why not
>>>    implement the VERSION structure (MS-NLMP 2.2.2.10)? Then use
>>>    LINUX_VERSION_* from <linux/version.h> or parse utsname()->release.
> That's a good idea. Let me explore that too.

No, it's not this is for windows version numbers.
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-nlmp/a211d894-21bc-4b8b-86ba-b83d0c167b00#Appendix_A_32

If you want to encode something you can use
2.2.2.2 Single_Host_Data

metze

Re: [PATCH] cifs: send workstation name during ntlmssp session setup

[Shyam Prasad N]

Hi Stefan,

Good points. Please read my replies inline below.

On Sun, Nov 7, 2021 at 4:19 PM Stefan Metzmacher <metze@samba.org> wrote:
>
>
> Hi Shyam,
>
> >>>> Please review this patch, and let me know what you think.
> >>>> Having this info in the workstation field of session setup helps
> >>>> server debugging in two ways.
> >>>> 1. It helps identify the client by node name.
> >>>> 2. It helps get the kernel release running on the client side.
> >>>>
> >>>> https://github.com/sprasad-microsoft/smb3-kernel-client/commit/d988e704dd9170c19ff94d9421c017e65dbbaac1.patch
> >>>
> >>> - AFAICS it doesn't consider runtime hostname changes. Is it important
> >>>    to keep track of it? Would changing it mid-auth steps break it
> >>>    somehow?
> > I think that's okay. AFAIK, that's only used for
> > debugging/troubleshooting purposes. So it doesn't need to be a 100%
> > accurate.
>
> That's not true, the workstation name is used for access checks.
>
> [MS-ADA3] 2.353 Attribute userWorkstations
> https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-ada3/ec941bac-bc77-48f3-a1cf-79d773a91b6b
>
I see.
So ideally we should be setting the FQDN of the client here. I don't
know if there's a way to get the FQDN from within the kernel.
I'll just set the utsname()->nodename for now. If someone has better
ideas, feel free to chime in.

> >>>
> >>> - I didn't understand the purpose of CIFS_DEFAULT_WORKSTATION_NAME. Why
> >>>    not simply use utsname()->nodename? Or even init_utsname()->nodename, which
> >>>    is supposed to be always valid.
> > I initially did not have the utsname changes. That idea was an afterthought.
> > Sure. I'll update the patch to fix this.
> >
> >>>
> >>> - Ditto for CIFS_MAX_WORKSTATION_LEN. utsname()->nodename has at most 65
> >>>    bytes (__NEW_UTS_LEN + 1) anyway. Perhaps using MAXHOSTNAMELEN from
> >>>    <asm/param.h> would be a more generic approach.
> >>>    (btw this is because nodename is the unqualified hostname, sans-domain)
> > Noted.
> >
> >>>
> >>> - Instead of setting workstation_name to "nodename:release", why not
> >>>    implement the VERSION structure (MS-NLMP 2.2.2.10)? Then use
> >>>    LINUX_VERSION_* from <linux/version.h> or parse utsname()->release.
> > That's a good idea. Let me explore that too.
>
> No, it's not this is for windows version numbers.
> https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-nlmp/a211d894-21bc-4b8b-86ba-b83d0c167b00#Appendix_A_32
>
> If you want to encode something you can use
> 2.2.2.2 Single_Host_Data
>
> metze

Thanks for the tip. I'll explore this option for a future fix.

-- 
Regards,
Shyam