Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support anonymous auth #13

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Oct 20, 2023

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
Copy link
Contributor Author

deitch commented Oct 20, 2023

Fixes #10

@deitch
Copy link
Contributor Author

deitch commented Oct 20, 2023

FYI, I tested it manually against a samba instance

@deitch
Copy link
Contributor Author

deitch commented Oct 25, 2023

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

@arashpayan
Copy link
Collaborator

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

@deitch deitch force-pushed the anonymous-auth branch 2 times, most recently from 844b804 to 8e8d28c Compare October 30, 2023 08:39
@deitch
Copy link
Contributor Author

deitch commented Oct 30, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

deitch commented Nov 5, 2023

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

internal/ntlm/common.go Outdated Show resolved Hide resolved
internal/ntlm/client.go Show resolved Hide resolved
@arashpayan
Copy link
Collaborator

Oops. I forgot to submit the review.

Signed-off-by: Avi Deitcher <avi@deitcher.net>
Copy link
Collaborator

@arashpayan arashpayan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@arashpayan arashpayan merged commit b0758ec into CloudSoda:main Nov 6, 2023
2 checks passed
@deitch deitch deleted the anonymous-auth branch November 7, 2023 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants