-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
websockets: Use AuthenticationMiddleware #2320
Conversation
with ESPAsyncWebServer 3.3.0, the setAuthentication() method became deprecated and a replacement method was provided which acts as a shim and uses the new middleware-based approach to setup authentication. in order to eventually apply a changed "read-only access allowed" setting, the setAuthentication() method was called periodically. the shim implementation each time allocates a new AuthenticationMiddleware and adds it to the chain of middlewares, eventually exhausting the memory. we now use the new middleware-based approach ourselves and only add the respective AuthenticatonMiddleware instance once to the respective websocket server instance. a regression where enabling unauthenticated read-only access is not applied until reboot is also fixed. all the AuthenticationMiddleware instances were never removed from the chain of middlewares when calling setAuthentication("", "").
when changing the security settings (disabling read-only access or changing the password), existing websocket connections are now closed, forcing the respective clients to authenticate (with the new password). otherwise, existing websocket clients keep connected even though the security settings now expect authentication with a (changed) password.
@schlimmchen you are setting Authentication to DIGEST Auth with Username + Realm ? I would prefer to set it to BASIC Auth and pre-calculate the base64 hash over See these signatures in // hash is the string representation of:
// base64(user:pass) for basic or
// user:realm:md5(user:realm:pass) for digest
bool authenticate(const char* hash);
bool authenticate(const char* username, const char* password, const char* realm = NULL, bool passwordIsHash = false); |
IMHO, we should not be concerned about this kind of overhead. Actually, I would support it if we change all HTTP endpoints to use the new AuthenticationMiddleware implementation, such that DIGEST authentication is the default everywhere. |
See our discussion in #2314 I would assume there are already quite a few projects that also use the Live API for remote displays etc. which would need to switch from BASIC Auth to DIGEST Auth. So I would not vote for this breaking change. I have explained my doubts regarding the benefits of DIGEST Auth (with fixed Client Nonce) over BASIC Auth on unsecure http connections in the original issue 😉 |
Will release a new version this evening and include this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I opened a question upstream with mathieucarbou regarding the performance of recalculating the hashes for every request here:
mathieucarbou/ESPAsyncWebServer#111
@@ -21,16 +21,30 @@ void WebApiWsConsoleClass::init(AsyncWebServer& server, Scheduler& scheduler) | |||
|
|||
scheduler.addTask(_wsCleanupTask); | |||
_wsCleanupTask.enable(); | |||
|
|||
_simpleDigestAuth.setUsername(AUTH_USERNAME); | |||
_simpleDigestAuth.setRealm("console websocket"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to set it to BASIC Auth here and rename it from _simpleDigestAuth
to _simpleBasicAuth
for backwards compatibility:
# _simpleDigestAuth.setRealm("console websocket");
_simpleBasicAuth.setAuthType(AuthenticationMiddleware::AuthType::AUTH_BASIC);
@@ -36,18 +36,31 @@ void WebApiWsLiveClass::init(AsyncWebServer& server, Scheduler& scheduler) | |||
|
|||
scheduler.addTask(_sendDataTask); | |||
_sendDataTask.enable(); | |||
_simpleDigestAuth.setUsername(AUTH_USERNAME); | |||
_simpleDigestAuth.setRealm("live websocket"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here ;)
it turns out that authentication was never implemented on OpenDTU-OnBattery-specific websocket connections. found while applying tbnobody#2320
|
||
void WebApiWsConsoleClass::reload() | ||
{ | ||
_ws.removeMiddleware(&_simpleDigestAuth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to remove the middleware: you can just call _simpleDigestAuth.setAuthMethod(NO_AUTH)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for commenting here, @mathieucarbou. Your suggestion makes a lot of sense. Could you answer us this: Do we even need multiple instances of the AuthenticationMiddleware if we want to protect all websockets the same? Can we have a single instance and add it to all servers? I guess there will be no problem with BASIC auth, but DIGEST possibly needs request-specific state, so I am not sure whether a static instance can just handle all connections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One single instance added to the server only will protected globally all handlers (http, ws, sse). But only the initial connection creation!
Eg, if you have some websocket connection opened, and change authentication settings, currently established connection can't be re-authenticated.
FYI I have reworked the Authc middleware - I will release a new version soon once the PR is merged: mathieucarbou/ESPAsyncWebServer#113 There will be some breaking changes but only in the enum and A method will be added to pre-compute the base64 hash or md5 hash for basic / digest auth. |
I also added a commit ti fix the memory leak (mathieucarbou/ESPAsyncWebServer@041565a) Please open an issue or PR (even better!) in the project also next time you see something like that ;-) Thanks! |
Thanks for your invitation to open an issue against your project in such a case. I would like to add that I did not feel that misusing a deprecated shim would warrant to open an issue and "complain". I think with that bugfix you went out of your way to keep the hassle for the users of your project to a minimum, despite good documentation about the deprecation. Good job 💪 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns. |
with ESPAsyncWebServer 3.3.0, the setAuthentication() method became deprecated and a replacement method was provided which acts as a shim and uses the new middleware-based approach to setup authentication. for unknown reasons, the setAuthentication() method was called periodically. the shim implementation each time allocates a new AuthenticationMiddleware and adds it to the chain of middlewares, eventually exhausting the memory.
we now use the new middleware-based approach ourselves and only add the respective AuthenticatonMiddleware instance once to the respective websocket server instance.
closes #2314.
closes #2316.
tests performed: