Adds support to NTLM Authentication #556

luizbafilho · 2018-03-22T15:08:09Z

Closes: #493

codecov-io · 2018-03-22T15:45:31Z

Codecov Report

Merging #556 into master will decrease coverage by 0.63%.
The diff coverage is 3.84%.

@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
- Coverage   62.89%   62.25%   -0.64%     
==========================================
  Files          96       97       +1     
  Lines        7217     7294      +77     
==========================================
+ Hits         4539     4541       +2     
- Misses       2429     2504      +75     
  Partials      249      249

Impacted Files	Coverage Δ
lib/netext/context.go	`36.36% <0%> (-63.64%)`	⬇️
lib/netext/http.go	`0% <0%> (ø)`
js/runner.go	`81.08% <100%> (ø)`	⬆️
js/modules/k6/http/http_request.go	`81.86% <100%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e50dea...661bd83. Read the comment docs.

na--

I have some very minor quibbles, the biggest of which is that I'm not sure that the code will measure the HTTP requests that set up the NTLM authentication, otherwise it LGTM

na-- · 2018-03-23T07:18:51Z

js/modules/k6/http/ntlm.go

+	// Use default round tripper if not provided
+	rt := n.RoundTripper
+	if rt == nil {
+		rt = http.DefaultTransport


I think we should return an error here instead of using the default transport, since I don't thing we would ever want to use it in k6 - it doesn't have netext.Dialer and the user-supplied configuration

na-- · 2018-03-23T07:29:19Z

js/modules/k6/http/ntlm.go

+
+	// Sending first request to get challenge data
+	req.Header.Set("Authorization", "NTLM TlRMTVNTUAABAAAABoIIAAAAAAAAAAAAAAAAAAAAAAA=")
+	res, err = rt.RoundTrip(req)


Hmm I think that data_sent and data_received will be correctly counted here, but I'm not sure if the other HTTP metrics that netext.Tracer saves will be

na-- · 2018-03-23T07:35:52Z

js/modules/k6/http/ntlm.go

+		return nil, err
+	}
+	if res.StatusCode != http.StatusUnauthorized {
+		return res, err


Since we know err is nil here, using return res, nil seems more readable. Also, I'm not sure if we want do directly return here anyway. If res.StatusCode is an error code like 404, 503, etc. - returning seems OK. But if res.StatusCode is 200 or another "good" code, when we expected to be challenged (since the use explicitly requested NTLM authentication), I think we may have to return some kind of error.

na-- · 2018-03-23T07:40:34Z

js/modules/k6/http/ntlm.go

+		return nil, errors.New("Empty WWW-Authenticate header")
+	}
+
+	challengeBytes, err := base64.StdEncoding.DecodeString(strings.Replace(ntlmChallenge, "NTLM ", "", -1))


strings.Replace(ntlmChallenge, "NTLM ", "", -1) seems like a bit of an overkill here. I'm not familiar with the protocol, but if the ntlmChallenge is expected to start with NTLM , shouldn't we check for that (and return an error if not present?) and then use something like strings.TrimPrefix or strings.Replace with 1 instead of -1?

na-- · 2018-03-23T07:43:16Z

js/modules/k6/http/ntlm.go

+}
+
+func getCredentialsFromHeader(header string) (string, string, error) {
+	credBytes, err := base64.StdEncoding.DecodeString(strings.Replace(header, "Basic ", "", -1))


Same here, strings.TrimPrefix() seems like a better fit than strings.Replace()

na-- · 2018-03-23T07:45:07Z

js/modules/k6/http/ntlm_test.go

+var challenges map[string]*ntlm.ChallengeMessage
+
+func TestGetCredentialsFromHeader(t *testing.T) {
+	user, pass, err := getCredentialsFromHeader("Basic Ym9iOnBhc3M=")


Not sure, but it may be worth it to test the parsing of brokenAuthorization headers. Theoretically this could come from our users directly, not from the Go standard library converting URL credentials to a header

Signed-off-by: Luiz Filho <luizbafilho@gmail.com>

We are now using a RoundTripper in order to reuse the same authenticated connection to comply with NTLM spec Signed-off-by: Luiz Filho <luizbafilho@gmail.com>

na-- · 2018-03-29T06:19:30Z

js/modules/k6/http/http_request_test.go

@@ -626,6 +635,20 @@ func TestRequestAndBatch(t *testing.T) {
 				assert.NoError(t, err)
 				assertRequestMetricsEmitted(t, state.Samples, "GET", sr("HTTPBIN_IP_URL/digest-auth/auth/bob/pass"), url, 200, "")
 			})
+			t.Run("ntlm", func(t *testing.T) {
+				ntlmServer := httptest.NewServer(http.HandlerFunc(ntlmHandler("bob", "pass")))


See the previous comment about spinning up new httptest servers

na-- · 2018-03-29T06:22:24Z

js/modules/k6/http/http_request_test.go

@@ -92,6 +98,9 @@ func assertRequestMetricsEmitted(t *testing.T, samples []stats.Sample, method, u
 func newRuntime(t *testing.T) (*testutils.HTTPMultiBin, *common.State, *goja.Runtime, *context.Context) {
 	tb := testutils.NewHTTPMultiBin(t)

+	ntlmServer := httptest.NewServer(http.HandlerFunc(ntlmHandler("bob", "pass")))


You don't have to spin up new test HTTP servers, you can add custom handlers in the already started HTTPMultiBin instance, like so. Besides, I don't think this is actually used anywhere, you create another identical test server in the subtest.

na-- · 2018-03-29T06:26:02Z

js/modules/k6/http/http_request_test.go

+				url := strings.Replace(ntlmServer.URL, "http://", "http://bob:pass@", -1)
+
+				_, err := common.RunString(rt, fmt.Sprintf(`
+				let res = http.request("GET", "%s", null, { auth: "ntlm" });


Can you expand this a bit? For example, test that a non-authenticated request fails with 403, then this one which works, then another one with wrong credentials, then this one again? Or something like that, not sure what the actual best way to test all the corner cases is.

I agree it would be good more tests, but to do that, requires me to implement the NTLM mock as the spec says. Right now it is a very naive implementation, just enough to give a basic validation, implement it properly it is not a trivial task and I'm not sure we should spend even more time in a feature that will be barely used.

But I'll try to add as much tests as possible without having to implement more stuff in ntlm mock

Yeah, very good points, it's not worth it for this 👍

na-- · 2018-03-29T06:28:23Z

js/modules/k6/http/http_request_test.go

+		if auths[r.RemoteAddr] {
+			data := "authenticated"
+			w.Header().Set("Content-Length", fmt.Sprint(len(data)))
+			fmt.Fprint(w, data)


I'm not sure, but I think that there should be a return here

na-- · 2018-03-29T06:29:55Z

js/modules/k6/http/http_request_test.go

+	challenges := make(map[string]*ntlm.ChallengeMessage)
+	auths := make(map[string]bool)
+	return func(w http.ResponseWriter, r *http.Request) {
+		if auths[r.RemoteAddr] {


If we have more NTLM tests in the future or a batch request in the current one, this will have a data race. Since this is a simple test handler, a global lock just in case should be fine I think

na-- · 2018-03-29T06:58:42Z

lib/netext/http.go

+	}
+}
+
+func (t *HTTPTransport) RoundTrip(req *http.Request) (res *http.Response, err error) {


To be honest, I'm a bit concerned that we have to use our own RoundTripper for authentication. On one hand, if we'll have this, fixing #562 should just mean figuring out how to properly collect and tag all of the samples. On the other hand, Go's documentation for RoundTripper explicitly states "RoundTrip should not attempt to handle higher-level protocol details such as redirects, authentication, or cookies.". I don't think it's too big of a deal, but it seems perfectly possible that we may be shooting ourselves in the foot for some currently unused or future Go http assumption or feature.

The only easy alternative I currently see though is to wrap the http.Client and its Transport, which is basically the same as this. The harder alternative would be to write our own client, which is not a trivial task...

I agree with you, but I couldn't find a different way. At least if it is not an NTLM request, everything else works as it is supposed to be.

Won't we plug the digest auth here as well at some point? So it doesn't make two requests every time?

Given that digest doesn't require authentication by connection, I guess we could instead of putting this logic inside the round tripper, we should do it where it is supposed to be, inside the client.

It might be confusing having two places with different authentications, but at least we are minimizing what we doing wrong.

It doesn't require authentication by connection, but if we want to have an optimized digest authentication, we'll probably have to cache the authorization header between different requests with the same domain and credentials. I have no idea what the best way to do that is...

To accomplish that, we do the same we did in ntlm but at the client level, it would require a custom Client. It can be as simple as the ntlm, check if it is digest, otherwise, do nothing and call the default implementation.

na-- · 2018-03-29T07:03:31Z

lib/netext/auth.go

+	"sync"
+)
+
+type AuthCache struct {


Is this actually used?

na-- · 2018-03-29T07:09:17Z

lib/netext/http.go

+	t.authCache[key] = value
+}
+
+func (t *HTTPTransport) getAuthCache(key string) (bool, bool) {


If this is only ever used for NTML, the API could be simpler, since the first bool will always be true if the second one is. If we plan to use it for Digest auth as well, we'll need the map to be more like the unused AuthCache struct.

Agree with you, once we decide to do the same to Digest, we should change that.

luizbafilho requested review from robingustafsson and na-- March 22, 2018 15:45

na-- reviewed Mar 23, 2018

View reviewed changes

luizbafilho added 7 commits March 27, 2018 13:25

Adding go-ntlm dependency

b7333d4

Adding support to NTLM Authentication

277a831

Signed-off-by: Luiz Filho <luizbafilho@gmail.com>

Reorganize ntlm tests

ff3e15c

Fixing unecessary conversion

138487f

Fixing unused method linter error

470a18a

Fixing NTLM Implementation

c68f4a1

We are now using a RoundTripper in order to reuse the same authenticated connection to comply with NTLM spec Signed-off-by: Luiz Filho <luizbafilho@gmail.com>

Adding missing ntlm server

321ce4b

luizbafilho force-pushed the feature/ntlm-auth branch from 27c6a4a to 321ce4b Compare March 27, 2018 16:40

na-- requested changes Mar 29, 2018

View reviewed changes

Cleaning up and adding a fail case to ntlm test

661bd83

na-- approved these changes Mar 29, 2018

View reviewed changes

luizbafilho merged commit 0bdf8d3 into master Mar 29, 2018

luizbafilho deleted the feature/ntlm-auth branch March 29, 2018 16:18

robingustafsson mentioned this pull request Aug 30, 2018

Fix redirect requests tracing #753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support to NTLM Authentication #556

Adds support to NTLM Authentication #556

luizbafilho commented Mar 22, 2018

codecov-io commented Mar 22, 2018 •

edited

Loading

na-- left a comment

na-- Mar 23, 2018

na-- Mar 23, 2018

na-- Mar 23, 2018

na-- Mar 23, 2018

na-- Mar 23, 2018

na-- Mar 23, 2018 •

edited

Loading

na-- Mar 29, 2018

na-- Mar 29, 2018

na-- Mar 29, 2018

luizbafilho Mar 29, 2018

luizbafilho Mar 29, 2018

na-- Mar 29, 2018

na-- Mar 29, 2018

na-- Mar 29, 2018

na-- Mar 29, 2018

luizbafilho Mar 29, 2018

na-- Mar 29, 2018

luizbafilho Mar 29, 2018

luizbafilho Mar 29, 2018

na-- Mar 29, 2018

luizbafilho Mar 29, 2018

na-- Mar 29, 2018

na-- Mar 29, 2018

luizbafilho Mar 29, 2018

Adds support to NTLM Authentication #556

Adds support to NTLM Authentication #556

Conversation

luizbafilho commented Mar 22, 2018

codecov-io commented Mar 22, 2018 • edited Loading

Codecov Report

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 22, 2018 •

edited

Loading

na-- Mar 23, 2018 •

edited

Loading