Support digest auth

[regan-karlewicz]

Addresses #19

I recently had a use case for digest auth and ended up implementing a Req plugin for it, so I decided to open a PR to integrate into Req after seeing the closed issue :)

[davydog187]

Hi @wojtekmach 👋 this came up at TV Labs and I encouraged @regan-karlewicz to open a PR. Let us know if you want to chat!

[wojtekmach]

Thank you for the PR! I left some comments below.

[wojtekmach]

We should not have two ways to do digest auth:

1. Req.get(url, auth: {:digest, "user:pass"})
2. Req.get(url, http_digest: [username: "user", password: "pass"])

My preference would be to use option 1, however if it's clunky and/or fragile to use the username:password notation (my understanding is we split it by :, do something with it and then join as username:realm:password), then we should support both of these:

auth: {:digest, "user:pass"}
auth: {:digest, username: "user", password: "pass"}

(And then a separate PR to support auth: {:basic, username: "user", password: "pass"} would be most welcome.)

WDYT?

If this sounds good, we should document it on Req.new (like you did) and Req.Steps.auth.

Btw, do you need to pass :count in your project? If not, let's not expose it and calculate nc internally.

[regan-karlewicz]

👍 Agreed on the single way to do digest auth, I'll make a change here.

I don't believe that using username:password and splitting is any more fragile than how basic auth is handled already, so I think let's use option 1. I am happy to contribute another change for explicit username and password values if you think we need it.

As far as count goes, you're right that we want to calculate this internally. If we want to avoid the 401/200 dance on every request, we would need a way to track the state internally (increment a count with each request for a specific server-side nonce.) Alternatively, we could accept the overhead that each request would receive a 401 response before making the authorized request.

I'm leaning towards the latter solution for the sake of simplicity and could implement the stateful solution later on if it comes up, but please do let me know what you think.

[regan-karlewicz]

I've updated the PR to do the stateless solution.

[wojtekmach]

Just two comments left!

[wojtekmach]

:count seems to be always 1? The only call site I found is:

        handle_challenge_reply(request, response,
          username: username,
          password: password
        )

which does not set :count.

[regan-karlewicz]

Hey, this is what I was mentioning before regarding the stateless vs stateful solution.

To use this field to properly pipeline requests, we would need to keep track of the number of requests made using each server nonce, and if the server sends a nextnonce value on the response in the Authentication-Info response header, update the nonce and reset the counter.

I'm happy to implement this logic, but we would need to determine the best way to persist state between requests. Do you have any recommendations here? I should note that if we don't implement this in this PR, it simply means that two requests will be made when using digest auth for each request. We could implement this as an optimization later.

[wojtekmach]

Sorry for ignorant question, but what's the use case for stateful solution?

My understanding is we'd always need to make at least two requests, the first will obviously 401 and then as per this implementation we will automatically make the second request using server's challenge etc to calculate the digest. And then I suppose we could keep the challenge etc around and we make yet another Req call, it could be then just one request. In other words:

resp1 = Req.get!(url, auth: {:digest, "user:pass"}) # always 2 requests
resp2 = Req.get!(url, auth: digest_with_extra_data) # can be just 1 request

Is that what's happening?

I suppose if one is making lots of requests in the small time span, I can see the benefit.

The way I'd see it is perhaps something like this:

1. {:digest, "user:pass"} is the same as {:digest, username: "user", password: "pass"}
2. Req.Response.get_www_authenticate(resp) returns [realm: ..., ...] | nil. This way, a user could semi-conveniently do: auth: {:digest, [username: ..., password: ...] ++ data_from_get_www_authenticate}.

WDYT?

But yeah, unless you need it for your app, I would wait.

[wojtekmach]

Ah, I suppose the response for the second request would not have www-authenticate header anymore so we'd have to store it for later and add API to grab it, perhaps something like Req.Response.get_auth_digest_data(resp) but yeah, not loving it 😅

[regan-karlewicz]

Yes, your understanding is correct about the use case here (reducing # of requests)

I do like the idea of a helper function to retrieve the data and passing it back in on subsequent requests, since it gives the user full control and doesn't require maintaining an internal state. It could retrieve the data needed for subsequent requests in regard to the current nonce and count (depending on whether a nextnonce was returned)

I agree, let's wait for now if that sounds good to you, if we need this later I can take a shot at implementing it :)

[wojtekmach]

Thank you!