-
Notifications
You must be signed in to change notification settings - Fork 1.4k
httpext: replace HTTP digest library #4599
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.
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
base: master
Are you sure you want to change the base?
Conversation
572b366
to
77bfb81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
// Actually make the HTTP request with the proper Authorization | ||
return t.originalTransport.RoundTrip(req) | ||
return digestTr.RoundTrip(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github.com/icholy/digest
will handle the 401 response and cache the digest challenge for us.
https://github.com/icholy/digest/blob/v1.1.0/transport.go#L141-L188
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @Juneezee 🙇
I think that we can drop the whole digest_transport.go
file and not have one more transport wrapper.
I wonder if we can get some tests to see that we do not get 401 each time , as that is one of the consequences of the original issue that we want fixed.
Reference: grafana#4599 (review) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@mstoykov I tried it, and it seems that the k6/js/modules/k6/http/request_test.go Lines 956 to 957 in dafd7f8
|
Will it be possible to support custom digest header set by js |
Reference: grafana#4599 (review) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
8d3e74a
to
d58e09e
Compare
Reference: grafana#4599 (review) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
d58e09e
to
b870206
Compare
Reference: grafana#4599 (review) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
b870206
to
3c1b834
Compare
I can not at this point look into this more in depth, but I would argue swithcing the implementation wihtout making it make less 401 is not actually beneficial. We have used the previous library for years and have had zero amounts of problems or issues reported AFAIK. To be honest I do not know if this is even used all that much. Swithcing the library at this point just adds potential for breakage and bugs. I think the problem is that we do not save the transport between invocations, which will potentially make it possible to cache this. |
@mstoykov I wonder if that is possible? The whole "first request may return 401" is part of the specification for digest auth. Its purpose is to get the challenge from the server and use that in the second request. From Postman docs (screenshot included too just in case the link becomes invalid in the future):
From RFC 7616 - HTTP Digest Access Authentication, Section 3.6 Digest Operation:
From RFC 7616 - HTTP Digest Access Authentication, Section 3.7 Security Protocol Negotiation:
|
Reference: grafana#4599 (review) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
3c1b834
to
1c72bf0
Compare
Reference: grafana#4599 (review) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
1c72bf0
to
3ac60bd
Compare
The current HTTP digest library, `github.com/Soontao/goHttpDigestClient`, has not been updated since March 2017 and is known to be buggy. This commit replaces it with `github.com/icholy/digest`, a more modern library that provides an `http.RoundTripper` implementation, allowing digest challenges to be reused. This aligns well with our use case. Closes grafana#800. Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Reference: grafana#4599 (review) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
3ac60bd
to
917eaa5
Compare
Sorry for the slow reply 🙇 It is possible - otherwise this would not be a usable case if for each request that needs to be authenticated you need to always go through the whole thing From the third paragraph in https://datatracker.ietf.org/doc/html/rfc7616#section-3.6
This is also what the whole digest transport you propose to use is doing - it is caching the header values and continues to give them until it gets requested again. The problem is that we make new of those transport on each request. This means that we will now have to keep the transport for digest for the duration of the ... iteration or maybe for the duration of the life of the VU. I guess we have to take into account no vu connection reuse. Partly because while this maybe isn't a connection it means that you will reuse authentication between connections. To be honest this looks like it might require some more options/parameters to be actually usefully configured. But in all cases we will need to keep the digest transport in between invocaitons to Hope this explains the problem better. |
What?
This PR replaces our current HTTP digest library
github.com/Soontao/goHttpDigestClient
withgithub.com/icholy/digest
.Why?
The current HTTP Digest library,
github.com/Soontao/goHttpDigestClient
, has not been updated since March 2017 and is known to be buggy.github.com/icholy/digest
is a newer library that provides anhttp.RoundTripper
implementation, allowing digest challenges to be reused. This aligns well with our use case.Checklist
make check
) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #800.