search

CloudSoda / go-smb2

Client implementation of the SMB 2 & 3 protocols
BSD 2-Clause "Simplified" License
13 stars 5 forks source link

support anonymous auth #13

Closed deitch closed 1 year ago

deitch commented 1 year ago

This PR adds support for anonymous auth, i.e. no provision of username and password. Of course, if the requested share does not allow it, it will return an auth error, which it should. If connecting to a share that allows anonymous access, it will work.

While doing so, I moved the Unmarshaling of the challenge message into its own function and struct. I needed it to make the Authenticate() function clearer to read, to figure out why something wasn't working. Once I figured it out, I left it there. If it bothers you, I can put it back to how it was before.

I extended the test TestClientServer() in ntlm_test.go. However, it appears that s.Authenticate() calls the server authentication directly, and our server does not know how to handle anonymous clients. So that is not a great test. I also am not sure how to update the server correctly to do this. Help would be appreciated.

deitch commented 1 year ago

Fixes #10

deitch commented 1 year ago

FYI, I tested it manually against a samba instance

deitch commented 1 year ago

Anything else needed for this, beyond fixing those tests? And any good suggestions how?

arashpayan commented 1 year ago

Hey @deitch. It's just been a very busy time at work. I'll be sure to review the PR this week.

deitch commented 1 year ago

OK, I restructured the server Authenticate(), and it handles anonymous correctly now. I checked, it only is used as part of ntlm_test.go, and that, in turn, is in internal/, so we are covered from all possible use cases. We might want to think about making it public at some point, but that is beyond this PR.

Ready for your review.

arashpayan commented 1 year ago

This looks good @deitch. Just one minor nitpick to address, and then I'll bring it in.

I tested the anonymous authentication against a local Samba server and it worked perfectly. After we merge this in, I'd like to add a test for it to the GitHub workflow.

deitch commented 1 year ago

What nitpick is that? I didn't see any other comment 🤷‍♂️

arashpayan commented 1 year ago

Oops. I forgot to submit the review.