* [PATCH] cifs: send workstation name during ntlmssp session setup @ 2021-11-05 19:22 Shyam Prasad N 2021-11-06 1:38 ` Enzo Matsumiya 0 siblings, 1 reply; 6+ messages in thread From: Shyam Prasad N @ 2021-11-05 19:22 UTC (permalink / raw) To: Steve French, CIFS, Paulo Alcantara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: send workstation name during ntlmssp session setup 2021-11-05 19:22 [PATCH] cifs: send workstation name during ntlmssp session setup Shyam Prasad N @ 2021-11-06 1:38 ` Enzo Matsumiya [not found] ` <CAH2r5mvQG0DFmMdzojH2u_w2=_9oGRV++AnEt_d7WJzj=-uTKA@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Enzo Matsumiya @ 2021-11-06 1:38 UTC (permalink / raw) To: Shyam Prasad N; +Cc: Steve French, CIFS, Paulo Alcantara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CAH2r5mvQG0DFmMdzojH2u_w2=_9oGRV++AnEt_d7WJzj=-uTKA@mail.gmail.com>]
* Re: [PATCH] cifs: send workstation name during ntlmssp session setup [not found] ` <CAH2r5mvQG0DFmMdzojH2u_w2=_9oGRV++AnEt_d7WJzj=-uTKA@mail.gmail.com> @ 2021-11-06 3:38 ` Shyam Prasad N 2021-11-06 7:02 ` Shyam Prasad N 2021-11-07 10:49 ` Stefan Metzmacher 0 siblings, 2 replies; 6+ messages in thread From: Shyam Prasad N @ 2021-11-06 3:38 UTC (permalink / raw) To: Steve French; +Cc: Enzo Matsumiya, CIFS, Paulo Alcantara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: send workstation name during ntlmssp session setup 2021-11-06 3:38 ` Shyam Prasad N @ 2021-11-06 7:02 ` Shyam Prasad N 2021-11-07 10:49 ` Stefan Metzmacher 1 sibling, 0 replies; 6+ messages in thread From: Shyam Prasad N @ 2021-11-06 7:02 UTC (permalink / raw) To: Steve French; +Cc: Enzo Matsumiya, CIFS, Paulo Alcantara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: send workstation name during ntlmssp session setup 2021-11-06 3:38 ` Shyam Prasad N 2021-11-06 7:02 ` Shyam Prasad N @ 2021-11-07 10:49 ` Stefan Metzmacher 2021-11-08 7:11 ` Shyam Prasad N 1 sibling, 1 reply; 6+ messages in thread From: Stefan Metzmacher @ 2021-11-07 10:49 UTC (permalink / raw) To: Shyam Prasad N, Steve French; +Cc: Enzo Matsumiya, CIFS, Paulo Alcantara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: send workstation name during ntlmssp session setup 2021-11-07 10:49 ` Stefan Metzmacher @ 2021-11-08 7:11 ` Shyam Prasad N 0 siblings, 0 replies; 6+ messages in thread From: Shyam Prasad N @ 2021-11-08 7:11 UTC (permalink / raw) To: Stefan Metzmacher; +Cc: Steve French, Enzo Matsumiya, CIFS, Paulo Alcantara 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-08 7:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05 19:22 [PATCH] cifs: send workstation name during ntlmssp session setup Shyam Prasad N
2021-11-06 1:38 ` Enzo Matsumiya
[not found] ` <CAH2r5mvQG0DFmMdzojH2u_w2=_9oGRV++AnEt_d7WJzj=-uTKA@mail.gmail.com>
2021-11-06 3:38 ` Shyam Prasad N
2021-11-06 7:02 ` Shyam Prasad N
2021-11-07 10:49 ` Stefan Metzmacher
2021-11-08 7:11 ` Shyam Prasad N
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.