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

[Shyam Prasad N <nspmangalore@gmail.com>]

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