bodgit / ntlmssp

Golang library implementing NTLM
https://godoc.org/github.com/bodgit/ntlmssp
BSD 3-Clause "New" or "Revised" License
13 stars 8 forks source link

Concurrency issue with winrm NTLM - http response error: 401 - invalid content type #51

Open CalypsoSys opened 11 months ago

CalypsoSys commented 11 months ago

When I am trying to connect to the same windows machine using NTLM authentication concurrently I get 401s for all requests but 1

found with integration with https://github.com/masterzen/winrm/issues/142

Test code below


package main

import (
    "bytes"
    "context"
    "fmt"
    "net/http"
    "sync"

    "github.com/bodgit/ntlmssp"
    ntlmhttp "github.com/bodgit/ntlmssp/http"
)

var (
    host    = "192.168.10.1"
    pwd     = "pwd"
    port    = 5985
    https   = false
    domain   = "my_domain"
    userName = "administrator"
)

func main() {
    Count := 10
    wg := new(sync.WaitGroup)
    wg.Add(Count)
    for i := 1; i <= Count; i++ {
        go ntlmAuth(i, host, domain, userName, pwd, port, https, wg)
    }

    wg.Wait()
    fmt.Println("done")
}

func ntlmAuth(i int, host string, domain string, user string, pwd string, port int, useHttps bool, wg *sync.WaitGroup) {
    defer wg.Done()

    ntlmClient, err := ntlmssp.NewClient(ntlmssp.SetUserInfo(user, pwd), ntlmssp.SetDomain(domain), ntlmssp.SetVersion(ntlmssp.DefaultVersion()))
    if err != nil {
        fmt.Println(i, " - Error in ntlmssp.NewClient: ", err)
        return
    }
    httpClient := &http.Client{}
    ntlmhttp, err := ntlmhttp.NewClient(httpClient, ntlmClient)
    if err != nil {
        fmt.Println(i, " - Error in ntlmhttp.NewClient: ", err)
        return
    }

    var scheme string
    if useHttps {
        scheme = "https"
    } else {
        scheme = "http"
    }

    // should use url.URL, but QD
    endpoint := fmt.Sprintf("%s://%s:%d/wsman", scheme, host, port)
    req, err := http.NewRequest("POST", endpoint, nil)
    if err != nil {
        fmt.Println("error in NewRequest", err)
        return
    }

    req.Header.Set("User-Agent", "WinRM client")
    req.Header.Set("Content-Length", "0")
    req.Header.Set("Content-Type", "application/soap+xml;charset=UTF-8")
    req.Header.Set("Connection", "Keep-Alive")

    resp, err := ntlmhttp.Do(req)
    if err != nil {
        fmt.Println("unknown error do", err)
        return
    }

    if resp.StatusCode != 200 {
        fmt.Println("http error", resp.StatusCode)
    } else {
        fmt.Println(i, " - ok")
    }
}
bodgit commented 11 months ago

I could imagine there being a problem if you tried to use the same *ntlmssp.Client in ten goroutines, but you're creating a new one in every goroutine so there's no shared state here between connections.

Have you ruled out that it's all NTLM services that are broken or is it just WinRM? WinRM does have concurrency limits which I think defaults to 5 so have you tried using 5 or fewer goroutines?

Also, I had no idea this package was now being used in another project. I'll cut a proper tag so you don't have to rely on commit hashes, even though it might just be a version 0.0.1.

NikunjPatel31 commented 11 months ago

@bodgit I have tried to use 5 or fewer goroutines with the above mentioned code and it still behaves the same. Still It gives error. Infact I tried to run to code using 2 goroutine.

We I try to make multiple conneciton with windows machine ( without NTLM / Normal connection ) using same IP address ( or same credentials ), I am able to do so. But it is not working when I am using NTLM.

bodgit commented 11 months ago

Try replacing this line:

httpClient := &http.Client{}

with:

httpClient := cleanhttp.DefaultClient()

and import github.com/hashicorp/go-cleanhttp. Or just pass nil for the httpClient, (as that's what this package does under the covers anyway):

https://github.com/bodgit/ntlmssp/blob/master/http/client.go#L39-L42

Does it work then?

CalypsoSys commented 11 months ago

@bodgit - that did not work I get

10  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
2  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
1  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
4  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
7  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
6  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
5  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
8  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
9  - Error in ntlmhttp.NewClient:  NTLM cannot work without keepalives
done

but if I use httpClient := cleanhttp.DefaultPooledClient()

it works for the NTLM test but still fails (but more connections work) in my WinRM test - there is a warning on DefaultPooledClient

// it can leak file descriptors over time. Only use this for transports that
// will be re-used for the same host(s).

also, thank you -

bodgit commented 11 months ago

Sorry, yes, you're right.

cleanhttp.DefaultPooledClient() returns the same thing as cleanhttp.DefaultClient(), just with keepalives disabled in the transport. That's a bug in my code then for the case of passing nil as the *http.Client, it should be using cleanhttp.DefaultPooledClient(), not sure how I didn't spot that!

So, the reason the code breaks is that httpclient := &http.Client{} returns a new client that still uses a global default transport from the net/http package. So all the goroutines have their own separate client, but each client is using the same single transport and so they're piggybacking on the same pool of connections, (because they're all going to the same host), rather than using their own separate connections. HTTP keepalives are required because the back & forth in the NTLM handshake needs to happen within the same TCP connection, and multiple NTLM handshakes happening in the same TCP connection is just going to confuse things, which will happen when each client is sharing a single transport.

This is the stated rationale for the cleanhttp package existing. If you're doing parallel connections to the same host, each connection needs to use its own client with its own transport to ensure they're completely isolated.

bodgit commented 11 months ago

What you probably also need is an http.Transport that also has MaxConnsPerHost set to 1 so you can guarantee each HTTP request will reuse the same connection.