Skip to content

Conversation

@ropnop
Copy link
Contributor

@ropnop ropnop commented Jun 7, 2020

Hello! Wanted to open this to start a conversation around adding NTLMSSP bind support to go-ldap/v3

I got this working by leveraging github.com/Azure/go-ntlmssp, which can handle negotiate and response message generation

To get the messages encoded properly, I also had to make some changes to github.com/go-asn1-ber/asn1-ber, which I submitted a PR for as well (go-asn1-ber/asn1-ber#27). Without that patch, ber.Encode was skipping over the negotiate and challenge-response messages.

In the end, this works on my AD domain with the following code:

func must(err error) {
	if err != nil {
		log.Fatal(err)
	}
}

func main() {

	l, err := ldap.DialURL("ldap://pdc01.lab.ropnop.com:389")
	if err != nil {
		log.Fatal(err)
	}
	defer l.Close()

	l.Debug = true

	err = l.NTLMBind("", "thoffman", "Password123")
	must(err)
}

We get a successful bind request and subsequent searches work.

Let me know what you think! I also have an idea to add support for authenticating with just an NT hash as well if you already have it, but that would require a change to the go-ntlmssp module as well.

Great library btw!

Copy link
Member

@johnweldon johnweldon 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, thank you.

  1. Please duplicate the code changes into the /bind.go file. (v3/bind.go should mirror bind.go for right now - I know it's a hassle - we need to figure out a long term strategy here)
  2. I don't have any clear suggestions on tests, but if you can think of a way to add some testing around your code to validate that it's working as expected, that'd be helpful.

@ropnop
Copy link
Contributor Author

ropnop commented Jun 17, 2020

Thanks for the feedback! I added comments, moved the import and copied the changes to the base bind.go. I'm still digging into how we can add tests I'll see if I can create something at least

@johnweldon
Copy link
Member

Looks good; when you're ready to merge you can rebase your branch on master

@ropnop
Copy link
Contributor Author

ropnop commented Jun 18, 2020

Merged master back in if you’re ready to merge this PR. The only way I can think of when it comes to testing is either mocking out an NTLM LDAP server or spinning one up in a Docker container for integration testing. Is that something you think is worthwhile?

@johnweldon
Copy link
Member

No, I don't want to try mocking out a server or spinning one up for the tests.

Thanks @ropnop

@johnweldon johnweldon merged commit c2c0783 into go-ldap:master Jun 18, 2020
@johnweldon
Copy link
Member

Released in version 3.2.0

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.

2 participants