Improve `HTTPDigest` implementation

[macheins]

Describe the bug

fastapi.security.HTTPDigest is actually not implementing HTTP Digest Access Authentication as specified by RFC 7616 or (the obsoleted) RFC 2617.

To Reproduce

Read the relevant RFCs or maybe try it out using curl --digest.

Expected behavior

Exactly as specified in RFC 7616.

[tiangolo]

Yeah, it is used internally to be integrated with OpenAPI, even though the implementation is just the basic stub. But I would accept PRs improving it.

[Kludex]

I'll take this.

[Kludex]

I'm not taking this. I did something on https://github.com/tiangolo/fastapi/pull/3071 and closed, maybe it helps...

[tfpgh]

Yeah, it is used internally to be integrated with OpenAPI, even though the implementation is just the basic stub. But I would accept PRs improving it.

Is this still open for improvement?

[tfpgh]

@tiangolo Hey, I've looked into implementing this and I'm not actually sure it's possible. To authenticate the user or even slightly abstract the protocol away from the underlying application code, HTTPDigest would need access to a store of usernames and hashed passwords. What are your thoughts?

@Kludex I would also love to hear about what you discovered in your initial attempt at implementing this.

[ordinary-jamie]

I'll take this!

[ordinary-jamie]

Hi @tiangolo didn't realise how much work was involved with this challenge-response negotiation. I made a PR #9825 to attempt to implement this and used curl --digest as the basic benchmark to ensure it's consistent with the RFC.

Because of the way that FastAPI provides security dependencies + the hashing involved with the negotiation, I couldn't think of an easy way to allow authentication, besides exposing an .authenticate(user, pass) -> bool method on the returned DI object.

Let me know what you think!

[ordinary-jamie]

Hey @Kludex, would love to hear your thoughts on the best approach for this change. Keen to get his closed. Cheers

[UpstreamData]

Would love to see this implemented as well, trying to replicate a device for testing that uses digest, and it's been quite the headache.

That being said, I tried getting #9825 working, and it appears it doesn't work with pydantic when the version is ^2, but I was able to get it working with 1.10.12, is this worth updating to the latest pydantic version?

[ordinary-jamie]

Thanks @UpstreamData! I will look into it

[ordinary-jamie]

@UpstreamData I've fixed the PR to properly support both pydantic ^2 and 1.10 -- was an issue with the model config; decided to do away with arbitrary types all together. Tested locally and the quick curl test works for both versions.

[UpstreamData]

Just getting back around to this now, it's working great with the new changes, but I have a suggestion to improve something I find confusing. Entirely up to you if you'd like to implement this, just an idea I had that makes my use case easier.

I find the use of auto_error confusing, as setting it to False basically means the whole process needs to be re-implemented on the application end. Curious if it would make more sense to have auto_error control erroring for invalid credentials, and possibly have another flag that turns off the more implicit errors, such as digest auth being None? This would allow for logging on the application end when a user login fails, but would allow the security layer to hide some of the complexity that has to go on in the background in order to implement the digest auth.

Here's an example of what I was thinking would work, but instead causes errors on the server -

import logging
import secrets

import uvicorn
from fastapi import HTTPException, FastAPI, Depends
from fastapi.security import HTTPDigest
from starlette.requests import Request
from starlette.responses import Response

security = HTTPDigest(realm="my website", auto_error=False)


async def auth(
    request: Request,
    response: Response,
):
    # create digest auth dialog
    digest = await security(request)

    # authenticate credentials
    if not await digest.authenticate("root", "root"):
        logging.info("WEB - login failed")  # never logs anything with auto_error=True
        raise HTTPException(status_code=401)

    logging.info("WEB - login succeeded")
    return response


app = FastAPI(dependencies=[Depends(auth)])


@app.get("/")
async def index():
    return {"hello": "world"}


uvicorn.run(app)