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.

Sign up for GitHub

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

Jump to bottom

Refactor HTTP digest authentication #800

Open
na-- opened this issue Oct 9, 2018 · 4 comments
Open

Refactor HTTP digest authentication #800

na-- opened this issue Oct 9, 2018 · 4 comments
Labels
bug high prio new-http issues that would require (or benefit from) a new HTTP API refactor

Comments

@na--
Copy link
Member

na-- commented Oct 9, 2018

Currently the HTTP digest authentication is part of the http.request() method, it would probably be better off (more readable and testable for sure) as a http.RoundTripper wrapper like these. This issue is a spin-off from the original HTTP request refactoring one.
@na-- na-- added bug and removed lower prio labels Jun 10, 2019
@na--
Copy link
Member Author

na-- commented Jun 10, 2019 •
edited
Loading

While testing the fixes for #1044 and #1041, I looked through the current HTTP digest authentication code and noticed that it prematurely returns from MakeRequest() when there's an error, unlike the rest of the code below. So, as a consequence, the response timings aren't properly populated...

https://github.com/loadimpact/k6/blob/607b63c4400c8b6852c7f51a0a6151f0bb8ec6b7/lib/netext/httpext/request.go#L317-L321

Additionally, something that I've omitted to mention in the original issue message - currently the HTTP digest authentication will always make 2 separate http requests.

@na--
Copy link
Member Author

na-- commented Jun 10, 2019

Connected Golang issue: golang/go#29409
The current digest authentication library we use is github.com/Soontao/goHttpDigestClient, some alternatives:

@na-- na-- added the high prio label Jun 10, 2019
@na-- na-- mentioned this issue Jun 10, 2019
Merged
imiric added a commit to imiric/k6 that referenced this issue Jul 15, 2019
@imiric
refactor(httpext): replace HTTP digest library
a88c89d 
This simplifies the k6 implementation, and avoids returning prematurely
on error, ensuring response timings are properly calculated.

Closes grafana#800
na-- added a commit that referenced this issue Aug 5, 2019
@na--
Fix response error handling and slightly improve the digest auth code
8c27b6b 
Previously, there was an uncaught error (sometimes leading to panics) in the automatic response body decompression. This patch handles these decompression errors and even assigns a custom error code for them. As a bonus, k6 now also properly handles errors due to improper redirects, or too many redirects, and tags the resulting metric samples accordingly.

By necessity, I had to also move the digest authentication handling to its own http.RoundTripper layer. This by no means solves the issues of #800, but it hopefully slightly improves the situation. A proper authentication cache and likely a different library still have to be used there eventually...
@na-- na-- mentioned this issue Aug 5, 2019
Merged
na-- added a commit that referenced this issue Aug 6, 2019
@na--
Fix response error handling and improve digest auth & http-debug code (…
46cdbe2 
…#1102)

Previously, there was an uncaught error (sometimes leading to panics) in the automatic response body decompression. This patch handles these decompression errors and even assigns a custom error code for them. As a bonus, k6 now also properly handles errors due to improper redirects, or too many redirects, and tags the resulting metric samples accordingly.

By necessity, I had to also move the digest authentication and http-debug handling to their own http.RoundTripper layers. This by no means solves any of the following issues:
 - #800
 - #986
 - #1042
 - #774

...but it hopefully slightly improves the situation. For the digest authentication, a proper authentication cache, and very likely a different library, still have to be used... And regarding `http-debug`, a discussion about how some or all the remaining issues can be fixed can be found here: #1102 (comment)
@na-- na-- added this to the v1.0.0 milestone Aug 27, 2019
@na-- na-- mentioned this issue Oct 21, 2019
Open
@na--
Copy link
Member Author

na-- commented Oct 21, 2019

There are some bugs with cookie handling in the NTLM authentication (#1210), which is currently handled in a very similar way to the digest authentication, so we should investigate if the same bug occurs here...

@na-- na-- mentioned this issue Jan 8, 2020
Closed
@na-- na-- modified the milestones: v1.0.0, v0.28.0 May 22, 2020
@na--
Copy link
Member Author

na-- commented May 22, 2020

Our current dependency seems to be dead, and has a bug that's can cause a panic... It assumes that a header value always contains key=value pairs: https://github.com/loadimpact/k6/blob/9586f2f4624548e40e9a0567cf821b5523489a36/vendor/github.com/Soontao/goHttpDigestClient/challenge.go#L37-L38

And funky server responses can cause k6 to panic with:

panic: runtime error: index out of range [1] with length 1 [recovered]
	panic: runtime error: index out of range [1] with length 1 [recovered]
	panic: runtime error: index out of range [1] with length 1

goroutine 57 [running]:
github.com/loadimpact/k6/vendor/github.com/dop251/goja.AssertFunction.func1.1(0xc0028b5cd8)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/runtime.go:1456 +0x98
panic(0xdfd920, 0xc002cb62e0)
	/usr/local/go/src/runtime/panic.go:679 +0x1b2
github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm).try.func1(0xc00049dc70, 0x0, 0xc0028b5ba8, 0x0, 0x0, 0x0, 0xc0028b5c00)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:373 +0x3b0
panic(0xdfd920, 0xc002cb62e0)
	/usr/local/go/src/runtime/panic.go:679 +0x1b2
github.com/loadimpact/k6/vendor/github.com/Soontao/goHttpDigestClient.NewChallenge(0xc002ccc090, 0x81, 0x10)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/Soontao/goHttpDigestClient/challenge.go:38 +0x26e
github.com/loadimpact/k6/vendor/github.com/Soontao/goHttpDigestClient.GetChallengeFromHeader(0xc00018a818, 0x0)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/Soontao/goHttpDigestClient/client.go:30 +0x5b
github.com/loadimpact/k6/lib/netext/httpext.digestTransport.RoundTrip(0x10a5aa0, 0xc001c44840, 0xc000c73a00, 0x0, 0x0, 0xc0028b4e58)
	/home/alpine/go/src/github.com/loadimpact/k6/lib/netext/httpext/digest_transport.go:69 +0x1a5
net/http.send(0xc000c73a00, 0x10a71a0, 0xc001723e70, 0x0, 0x0, 0x0, 0xc001770b90, 0xc0028b4b40, 0x2, 0x8)
	/usr/local/go/src/net/http/client.go:250 +0x443
net/http.(*Client).send(0xc001c44a50, 0xc000c73a00, 0x0, 0x0, 0x0, 0xc001770b90, 0x1, 0x2, 0x0)
	/usr/local/go/src/net/http/client.go:174 +0xfa
net/http.(*Client).do(0xc001c44a50, 0xc000c73a00, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/http/client.go:641 +0x3ce
net/http.(*Client).Do(...)
	/usr/local/go/src/net/http/client.go:509
github.com/loadimpact/k6/lib/netext/httpext.MakeRequest(0x10b8a80, 0xc001c44270, 0xc000e08f00, 0x0, 0x0, 0x0)
	/home/alpine/go/src/github.com/loadimpact/k6/lib/netext/httpext/request.go:317 +0x7c1
github.com/loadimpact/k6/js/modules/k6/http.(*HTTP).Request(0xc00011a000, 0x10b8a80, 0xc001c44270, 0xe921d4, 0x3, 0x10c5760, 0xc002482a00, 0xc001cca7e0, 0x2, 0x2, ...)
	/home/alpine/go/src/github.com/loadimpact/k6/js/modules/k6/http/request.go:110 +0x19d
github.com/loadimpact/k6/js/modules/k6/http.(*HTTP).Get(0xc00011a000, 0x10b8a80, 0xc001c44270, 0x10c5760, 0xc002482a00, 0xc0017235b0, 0x1, 0x1, 0x0, 0x0, ...)
	/home/alpine/go/src/github.com/loadimpact/k6/js/modules/k6/http/request.go:52 +0x15b
reflect.Value.call(0xe364e0, 0xc00011a000, 0x1213, 0xe97b43, 0x9, 0xc001096780, 0x3, 0x3, 0xb, 0xdfca40, ...)
	/usr/local/go/src/reflect/value.go:460 +0x5f6
reflect.Value.CallSlice(0xe364e0, 0xc00011a000, 0x1213, 0xc001096780, 0x3, 0x3, 0x194, 0xc000fc26f0, 0x8c84a3)
	/usr/local/go/src/reflect/value.go:334 +0xb4
github.com/loadimpact/k6/js/common.Bind.func1(0x10c5580, 0xc000f7d060, 0xc000c14300, 0x2, 0x8, 0x1e6fcf496b80abe, 0x40c6d8)
	/home/alpine/go/src/github.com/loadimpact/k6/js/common/bridge.go:241 +0x780
github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm)._nativeCall(0xc00049dc70, 0xc0024e89a0, 0x2)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:1835 +0x291
github.com/loadimpact/k6/vendor/github.com/dop251/goja.call.exec(0x2, 0xc00049dc70)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:1819 +0x4a2
github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm).run(0xc00049dc70)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:289 +0x99
github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*funcObject).Call(0xc000f6bb00, 0x10c5bc0, 0x19796e0, 0xc001723220, 0x1, 0x1, 0x10c5bc0, 0x19796e0)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/func.go:130 +0x2cc
github.com/loadimpact/k6/vendor/github.com/dop251/goja.AssertFunction.func1.2()
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/runtime.go:1461 +0x96
github.com/loadimpact/k6/vendor/github.com/dop251/goja.(*vm).try(0xc00049dc70, 0xc000fc2c50, 0x0)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/vm.go:379 +0x147
github.com/loadimpact/k6/vendor/github.com/dop251/goja.AssertFunction.func1(0x10c5bc0, 0x19796e0, 0xc001723220, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0)
	/home/alpine/go/src/github.com/loadimpact/k6/vendor/github.com/dop251/goja/runtime.go:1460 +0x13a
github.com/loadimpact/k6/js.(*VU).runFn(0xc0006d5cc0, 0x10b89c0, 0xc0019ee380, 0xc000ab49c0, 0x10b8901, 0xc000f7d280, 0xc001723220, 0x1, 0x1, 0xc00007a0c0, ...)
	/home/alpine/go/src/github.com/loadimpact/k6/js/runner.go:477 +0x35e
github.com/loadimpact/k6/js.(*VU).RunOnce(0xc0006d5cc0, 0x10b89c0, 0xc0019ee380, 0x0, 0x0)
	/home/alpine/go/src/github.com/loadimpact/k6/js/runner.go:427 +0x277
github.com/loadimpact/k6/core/local.(*vuHandle).run(0xc0015aff40, 0xc00007e070, 0xc00007a480, 0xc00012c600)
	/home/alpine/go/src/github.com/loadimpact/k6/core/local/local.go:70 +0x152
github.com/loadimpact/k6/core/local.(*Executor).scale.func1(0xc0015aff40, 0xc000d47b00, 0xc00007a480, 0xc00012c600)
	/home/alpine/go/src/github.com/loadimpact/k6/core/local/local.go:349 +0x4d
created by github.com/loadimpact/k6/core/local.(*Executor).scale
	/home/alpine/go/src/github.com/loadimpact/k6/core/local/local.go:348 +0x328

@na-- na-- mentioned this issue Jul 23, 2020
Merged
@na-- na-- modified the milestones: v0.28.0, v1.0.0 Sep 8, 2020
@na-- na-- mentioned this issue Jan 29, 2021
Closed
@na-- na-- mentioned this issue Feb 25, 2021
Merged
@na-- na-- added the new-http issues that would require (or benefit from) a new HTTP API label Oct 18, 2021
@na-- na-- modified the milestones: v1.0.0, TBD Nov 9, 2022
@codebien codebien removed this from the TBD milestone Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug high prio new-http issues that would require (or benefit from) a new HTTP API refactor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@codebien @na--