Skip to content

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented Mar 1, 2025

What?

This PR replaces our current HTTP digest library github.com/Soontao/goHttpDigestClient with github.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 an http.RoundTripper implementation, allowing digest challenges to be reused. This aligns well with our use case.

Checklist

  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests for my changes.
  • I have run linter and tests locally (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.

  • I have added the correct milestone and labels to the PR.
  • I have updated the k6-documentation: grafana/k6-docs#PR-NUMBER
  • I have updated the TypeScript definitions: grafana/k6-DefinitelyTyped#PR-NUMBER
  • I have updated the release notes: link

Related PR(s)/Issue(s)

Closes #800.

@Juneezee Juneezee force-pushed the refactor/http-digest branch from 572b366 to 77bfb81 Compare March 1, 2025 17:25
Copy link
Contributor Author

@Juneezee Juneezee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have performed a self-review of my changes.

/cc @na--, author of Issue #800

}

// Actually make the HTTP request with the proper Authorization
return t.originalTransport.RoundTrip(req)
return digestTr.RoundTrip(req)
Copy link
Contributor Author

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

@Juneezee Juneezee marked this pull request as ready for review March 1, 2025 17:51
@Juneezee Juneezee requested a review from a team as a code owner March 1, 2025 17:51
@Juneezee Juneezee requested review from mstoykov and inancgumus and removed request for a team March 1, 2025 17:51
Copy link
Contributor

@mstoykov mstoykov left a 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.

Juneezee added a commit to Juneezee/k6 that referenced this pull request Mar 4, 2025
Reference: grafana#4599 (review)
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee
Copy link
Contributor Author

Juneezee commented Mar 4, 2025

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.

@mstoykov I tried it, and it seems that the 401 Unauthorized case is unavoidable. The screenshot below shows the debugger attached to the TestRequest/Params/digest/auth/success test case, where we can see that the response status code is 401.

t.Run("digest", func(t *testing.T) {
t.Run("success", func(t *testing.T) {

digest-401-debugger

@Juneezee Juneezee requested a review from mstoykov March 4, 2025 09:33
@kapsh
Copy link

kapsh commented Mar 7, 2025

Will it be possible to support custom digest header set by js Params.auth? I encountered service that sends challenge in non-standard x-something-authenticate and I'd like to use k6 to test it.

inancgumus pushed a commit to Juneezee/k6 that referenced this pull request Mar 7, 2025
Reference: grafana#4599 (review)
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@inancgumus inancgumus force-pushed the refactor/http-digest branch from 8d3e74a to d58e09e Compare March 7, 2025 20:15
Juneezee added a commit to Juneezee/k6 that referenced this pull request Mar 9, 2025
Reference: grafana#4599 (review)
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee Juneezee force-pushed the refactor/http-digest branch from d58e09e to b870206 Compare March 9, 2025 05:04
inancgumus pushed a commit to Juneezee/k6 that referenced this pull request Mar 10, 2025
Reference: grafana#4599 (review)
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@inancgumus inancgumus force-pushed the refactor/http-digest branch from b870206 to 3c1b834 Compare March 10, 2025 16:37
@mstoykov
Copy link
Contributor

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.

@Juneezee
Copy link
Contributor Author

wihtout making it make less 401

@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):

the client sends a first request to the API, and the server responds back with details. Response details include a number that can be used only once (a nonce), a realm value, and a 401 Unauthorized response. You then send back an encrypted array of data, including a username and password combined with the data received from the server in the first request.

2025-03-12_18-47

From RFC 7616 - HTTP Digest Access Authentication, Section 3.6 Digest Operation:

the server MAY return a 401 response with a new nonce value in the WWW-Authenticate header field, causing the client to retry the request;

From RFC 7616 - HTTP Digest Access Authentication, Section 3.7 Security Protocol Negotiation:

When a server receives a request to access a resource, the server might challenge the client by responding with "401 Unauthorized" response and include one or more WWW-Authenticate header fields.

Juneezee added a commit to Juneezee/k6 that referenced this pull request Mar 13, 2025
Reference: grafana#4599 (review)
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee Juneezee force-pushed the refactor/http-digest branch from 3c1b834 to 1c72bf0 Compare March 13, 2025 03:52
Juneezee added a commit to Juneezee/k6 that referenced this pull request Mar 14, 2025
Reference: grafana#4599 (review)
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@Juneezee Juneezee force-pushed the refactor/http-digest branch from 1c72bf0 to 3ac60bd Compare March 14, 2025 19:25
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>
@Juneezee Juneezee force-pushed the refactor/http-digest branch from 3ac60bd to 917eaa5 Compare March 23, 2025 08:58
@mstoykov
Copy link
Contributor

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

The authentication session lasts until the client receives another
WWW-Authenticate challenge from any server in the protection space.
A client SHOULD remember the username, password, nonce, nonce count,
and opaque values associated with an authentication session to use to
construct the Authorization header field in future requests within
that protection space.

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 http.get and co.

Hope this explains the problem better.

@Juneezee Juneezee marked this pull request as draft March 29, 2025 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor HTTP digest authentication
3 participants