Reset Authorization header on proxied request #2259
Conversation
|
Is there anything more I need to do to get this reviewed? |
This comment has been minimized.
This comment has been minimized.
bytespider
force-pushed
the
fix-mosquitto-authentication
branch
from
6f59764
to
4e6479e
Compare
bytespider
force-pushed
the
fix-mosquitto-authentication
branch
from
4e6479e
to
11d69ac
Compare
|
@MartinHjelmare is anyone interested in this? I believe it's the correct solution as it brings the known behaviour from 5.x back into 6.x devices could no longer authenticate if they contained non-alphanumeric characters. |
| @@ -35,6 +35,7 @@ http { | |||
|
|
|||
| location /authentication { | |||
| proxy_set_header X-Supervisor-Token "{{ env "SUPERVISOR_TOKEN" }}"; | |||
| proxy_set_header Authorization ""; | |||
There was a problem hiding this comment.
This is incorrect, this will break HA authentication.
There was a problem hiding this comment.
@frenck can you please elaborate how?
As far as the code state when this was developed, the code checked first the Basic Auth, then the parameters that were passed in the URL.
HA uses Mosquitto Password and ACL files and doesn't authenticate via HTTP from this addon afaik.
There was a problem hiding this comment.
@frenck wondering if you saw my reply to your last response?
There was a problem hiding this comment.
@bytespider I missed that indeed. Sorry about that.
and doesn't authenticate via HTTP from this addon afaik.
It does :) It authenticates against Home Assistant with Home Assistant user accounts.
There was a problem hiding this comment.
Okay, thank you.
I'll go through the code again as I clearly missed something. I thought I understood it.
There was a problem hiding this comment.
This is true. However Basic Auth only allows for a limited set of characters, whilst Mosquitto usernames and passwords are more diverse.
What would the impact of retiring Basic Auth from the Supervisor Auth API be?
I wonder if it's worth having the Auth Plugin control which method is sent to the backend HTTP server? @pvizeli thoughts?
There was a problem hiding this comment.
What would the impact of retiring Basic Auth from the Supervisor Auth API be?
It is not about impact, it is about where this should be fixed. I feel like we are now working around a problem that shouldn't be "fixed" here, but on another place.
There was a problem hiding this comment.
Sorry perhaps I was unclear, I was suggesting that one option was perhapst that the Supervisory API no longer supports Basic Auth as a way or authorizing user credentials since it cannot handle the full set of characters supported by HA usernames and passwords.
There was a problem hiding this comment.
I was suggesting that one option was perhapst that the Supervisory API no longer supports Basic Auth
It was introduced as the last of all authentication handling and actually used by almost all add-ons.
So thinking about the specifications and limitations, this is about the colon : I guess.
I suggest ensuring we don't accept colons in usernames. Reason why: This is making it work for this special case, but that doesn't solve the case that Home Assistant currently allows for it (apparently) and the Supervisor can't handle it (apparently).
TL;DR; this PR works around an issue, and is not address or fixing the greater problem behind it. Therefore, I don't think this PR should be merged.
There was a problem hiding this comment.
Disappointing conclusion, but thank you.
Home Assistant allows a much richer set of characters for username, however the Mosquitto auth plugin sends both Basic Auth and a URL encoded parameter string.
HA will parse the Basic Authentication header first and fail, while the later URL encoded string authorization will not.
In order to skip the more restrictive Basic Authentication check, I've updated the Nginx proxying to remove that header.
This fixes #2014