Azure / go-ntlmssp

NTLM/Negotiate authentication over HTTP
MIT License
189 stars 67 forks source link

NTLM HTTP Auth doesn't work (wrong negotiation implementation)? #1

Closed ddelazerda closed 8 years ago

ddelazerda commented 8 years ago

Hi, I think there are few issues with the negotiator. Here's roughly how I'm trying to use your library:

package main

import (
    "fmt"
    "github.com/paulmey/go-ntlmssp"
    "log"
    "net/http"
    "net/http/httputil"
)

func dumpResponse(r *http.Response, prefix string) {
    dump, err := httputil.DumpResponse(r, true)
    if err != nil {
        log.Fatal(err)
    }
    fmt.Printf("%s %s\n", prefix, string(dump))
}

func main() {
    someURL := "http://example.com/some/endpoint/behind/ntlm.aspx"
    c := &http.Client{
        Transport: ntlmssp.Negotiator{
            &http.Transport{},
        },
    }
    req, _ := http.NewRequest("GET", someURL, nil)
    req.SetBasicAuth("someuser", "somepass")
    res, err := c.Do(req)
    if err != nil {
        log.Fatal(err)
    }
    dumpResponse(res, "Page retrived after NTLM authentication:\n")
}

This code doesn't work. It panics with a nil pointer de-reference and after fixing that in negotiator.go, it doesn't correctly authenticate. After making the changes in this diff it works as expected. What do you think?

paulmey commented 8 years ago

Negotiate and NTLM are different authentication methods. Negotiate can turn out to be NTLM in the end, but it can also be Kerberos. Anyways. This lib was a very minimal implementation to get WinRM working with the default settings on Azure. (See https://github.com/masterzen/winrm#pluggable-authentication-example-negotiatentlm-authentication) That said, I'm definitely open to making this more generic and usable. :smile: We're going to move this repo to the Azure org, since we're going to have the packer builder depend on it, but we can move it forward there. Your diff has some pretty straightforward improvements with regards to error handling for one. :+1:

paulmey commented 8 years ago

Oh and sorry for not paying attention to this repo for a couple of months...

ddelazerda commented 8 years ago

Hey, I still think there's an issue in the negotiator code. Before res.Body.Close() on lines 68 and 96 you still need to drain the res.Body content or else the connection won't be reused see the comment on my diff in the first post. After adding the code to drain res.Body the code in this repo works.

paulmey commented 8 years ago

I only recently learned that one was to supposed to drain connections for proper connection reuse. I'll take a stab at it...

SleepyBrett commented 8 years ago

I'm extra dense today. Do you guys have a fully working example of this thing? The code above does not seem to work for me.

paulmey commented 8 years ago

This is the only place we use it: https://github.com/mitchellh/packer/blob/f908e184831348d1d77eb2f347201c6542180084/builder/azure/arm/config.go#L279

SleepyBrett commented 8 years ago

@paulmey I'm not really up on the whole NTLM ecosystem, but what I'm seeing in this library is looking for response headers like WWW-Authenticate: Negotiate <base 64ed foo>, what I'm seeing is WWW-Authenticate: NTLM <base64ed foo>. Pointers, is this an older version of NTLM?

UPDATE: Turns out with a few tweaks ( mostly grabbing that final if resauth.IsNegotiate() block and testing for NTLM and setting Authorization NTLM instead makes it work.

I cleaned up my hack a bit, if you would like me to pull request it back in let me know.

paulmey commented 8 years ago

@SleepyBrett, cool that you got it to work. The docs on this protocol are pretty 'light'. PR's are always welcome, especially if they make this lib usable in more cases! Thanks in advance!