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

[feat] checkBasicAuthentication calculates the base64 encoded hash for username:password everytime #111

Closed
stefan123t opened this issue Sep 30, 2024 · 3 comments · Fixed by #113
Assignees
Labels

Comments

@stefan123t
Copy link

stefan123t commented Sep 30, 2024

Wouldn't it be better to calculate the base64 encoded hash for username:password only once, when you modify it using setUsername and setPassword ?

I have seen in https://github.com/mathieucarbou/ESPAsyncWebServer/blob/main/src/WebAuthentication.cpp#L34-67 it is calculated for every WebRequest to the same AuthenticationMiddleware again.

Before it is actually memcmp compared in Line 59:

... memcmp(hash, encoded, encodedLen) == 0
@stefan123t stefan123t added the question Further information is requested label Sep 30, 2024
@mathieucarbou
Copy link
Owner

Hello @stefan123t : I've read the upstream discussion in OpenDTU project. I love this project:-) I have one setup myself ;-)

So for your question, it works as before, no change for that.

The issue with your proposal is that the authentication method (basic or digest) is decided by the request and not known in advance. If you test with curl for example, you can access a protected endpoint either with a basic or digest authentication headers

The authentication method that is passed as parameter of the middleware is, like before, the authentication method that the server will use in its reply in case the authentication fails, in order to hint the browser (or curl).

I agree with your that considering now that the introduction of a middleware allows for more dynamic setup and also memory saving, maybe an option could be to either precomputed both hash, or just the one in relation to authentication method.

In any case I will have a look this evening if this can be quickly done.

@stefan123t
Copy link
Author

stefan123t commented Sep 30, 2024

Salut @mathieucarbou thanks for your interest and reply and thanks for maintaining EHCache 😉

For Basic Auth the resulting _hash / encoded base64("<username>:<password>") may be stored in the AuthenticationMiddleware.

Whereas for Digest Auth it may depend a bit on how much of the Digest Auth details have been implemented in the ESPAsyncWebServer.

I am not that familiar with Digest Auth, expecially when it comes to the advertising of the nonce / cnonce using a Server response.
I assume that the Server would have to make sure to announce a new nonce at least for every 401 Unauthenticated and the Client a new cnonce and an increased nc hex-counter value for each request, in order to prevent CORS and other Session high-jacking methods it should protect against. There is a quite detailled write-up on Wikipedia for more details: https://en.wikipedia.org/wiki/Digest_access_authentication

You can see that it makes use of _hash1 and _hash2, which could be stored in the AuthenticationManager and only the second _hash2 may need to be recomputed per request / method-url handler. Just like an Apache WebServer also stores _hash1 as part of the .htdigest file in the <username>:<realm>:<_hash1> format, here _hash1 is the md5sum( "username:realm:password"). And _hash2 is the md5sum("<method>:</url/path/resource>").
Only resulting the response is probably calculated new from request to request, i.e. md5sum("<_hash1>:<nonce>:<nc>:<cnonce>:<qop>:<_hash2>"), mostly because nc, cnonce and nonce may change over time, with nc being the most frequent IMHO, cnonce per Client and nonce less frequent.

@mathieucarbou
Copy link
Owner

Yes, when simulating with one of the curl command in the project:

curl -v -X GET -H "x-remove-me: value" -H "origin: http://192.168.4.1" --digest -u user:password http://192.168.4.1/middleware/auth-complex

curl issues a request first to get the tokens, then create its second request

❯  clear && curl -v -X GET -H "x-remove-me: value" -H "origin: http://192.168.4.1" --digest -u user:password  http://192.168.4.1/middleware/auth-complex

Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 192.168.4.1:80...
* Connected to 192.168.4.1 () port 80
* Server auth using Digest with user 'user'
> GET /middleware/auth-complex HTTP/1.1
> Host: 192.168.4.1
> User-Agent: curl/8.10.0
> Accept: */*
> x-remove-me: value
> origin: http://192.168.4.1
> 
* Request completely sent off
< HTTP/1.1 401 Unauthorized
< Connection: close
< WWW-Authenticate: Digest realm="asyncesp", qop="auth", nonce="795d0e2e77f0bd1ac56d88a223e30cc8", opaque="0bc073c1bf61fa0ae678fa5892cfd2a6"
< Access-Control-Allow-Origin: http://192.168.4.1
< Access-Control-Allow-Methods: POST, GET, OPTIONS, DELETE
< Access-Control-Allow-Headers: X-Custom-Header
< Access-Control-Allow-Credentials: false
< Access-Control-Max-Age: 600
< Accept-Ranges: none
< Content-Length: 0
< 
* shutting down connection #0
* Issue another request to this URL: 'http://192.168.4.1/middleware/auth-complex'
* Hostname 192.168.4.1 was found in DNS cache
*   Trying 192.168.4.1:80...
* Connected to 192.168.4.1 () port 80
* Server auth using Digest with user 'user'
> GET /middleware/auth-complex HTTP/1.1
> Host: 192.168.4.1
> Authorization: Digest username="user", realm="asyncesp", nonce="795d0e2e77f0bd1ac56d88a223e30cc8", uri="/middleware/auth-complex", cnonce="MDQwMDJkYmUyZDhmZDlmZTQ5Y2Y2MzdmNTZhNDJmOWQ=", nc=00000001, qop=auth, response="ce04170dd964a81c25ee22884446d32a", opaque="0bc073c1bf61fa0ae678fa5892cfd2a6"
> User-Agent: curl/8.10.0
> Accept: */*
> x-remove-me: value
> origin: http://192.168.4.1
> 
* Request completely sent off
< HTTP/1.1 200 OK
< Connection: close
< X-Rate-Limit: 200
< Access-Control-Allow-Origin: http://192.168.4.1
< Access-Control-Allow-Methods: POST, GET, OPTIONS, DELETE
< Access-Control-Allow-Headers: X-Custom-Header
< Access-Control-Allow-Credentials: false
< Access-Control-Max-Age: 600
< Accept-Ranges: none
< Content-Length: 30
< Content-Type: text/plain
< 
* shutting down connection #1
Hello Mathieu with role: staff

With basic auth, this is quite straightforward:

❯  curl -v -X GET -H "x-remove-me: value" -H "origin: http://192.168.4.1" -u admin:admin  http://192.168.4.1/middleware/auth-basic
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 192.168.4.1:80...
* Connected to 192.168.4.1 () port 80
* Server auth using Basic with user 'admin'
> GET /middleware/auth-basic HTTP/1.1
> Host: 192.168.4.1
> Authorization: Basic YWRtaW46YWRtaW4=
> User-Agent: curl/8.10.0
> Accept: */*
> x-remove-me: value
> origin: http://192.168.4.1
> 
* Request completely sent off
< HTTP/1.1 200 OK
< Connection: close
< Access-Control-Allow-Origin: http://192.168.4.1
< Access-Control-Allow-Methods: POST, GET, OPTIONS, DELETE
< Access-Control-Allow-Headers: X-Custom-Header
< Access-Control-Allow-Credentials: false
< Access-Control-Max-Age: 600
< Accept-Ranges: none
< Content-Length: 13
< Content-Type: text/plain
< 
* shutting down connection #0
Hello, world!

So I will try to improve and also refactor the code a little (breaking change in authc middleware, to align it with the Arduino WebServer enums and capabilities.

@mathieucarbou mathieucarbou removed the question Further information is requested label Sep 30, 2024
mathieucarbou added a commit that referenced this issue Sep 30, 2024
- to align methods and enum with PsychicHttp and Arduino WebServer
- to support hash
- to pre-compute base64 / digest hash to speed up requests
Closes #111
@mathieucarbou mathieucarbou changed the title [Q] checkBasicAuthentication calculates the base64 encoded hash for username:password everytime [feat] checkBasicAuthentication calculates the base64 encoded hash for username:password everytime Sep 30, 2024
mathieucarbou added a commit that referenced this issue Sep 30, 2024
- to align methods and enum with PsychicHttp and Arduino WebServer
- to support hash
- to pre-compute base64 / digest hash to speed up requests
Closes #111
mathieucarbou added a commit that referenced this issue Oct 1, 2024
- to align methods and enum with PsychicHttp and Arduino WebServer
- to support hash
- to pre-compute base64 / digest hash to speed up requests
Closes #111
mathieucarbou added a commit that referenced this issue Oct 1, 2024
- to align methods and enum with PsychicHttp and Arduino WebServer
- to support hash
- to pre-compute base64 / digest hash to speed up requests
Closes #111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants