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.

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

httpext: replace HTTP digest library #4599

Open
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
@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
@inancgumus inancgumus force-pushed the refactor/http-digest branch from 8d3e74a to d58e09e Compare March 7, 2025 20:15
Juneezee added 2 commits March 9, 2025 13:03
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 <[email protected]>
@Juneezee Juneezee force-pushed the refactor/http-digest branch from d58e09e to b870206 Compare March 9, 2025 05:04
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