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

Allow GET method on /auth with BasicAuth #2407

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

zeehio
Copy link
Contributor

@zeehio zeehio commented Jan 7, 2021

Proposed change

nginx has a convenient feature named Authentication Based on Subrequest Result which allows to grant/deny access using basic authentication based on the result of a subrequest. That nginx module however only works with GET requests.

This Pull Requests allows users to provide auth credentials using Basic Auth in a GET request. This is convenient because it is then possible to use nginx to authenticate a third party app. Within the nginx server directive an add-on developer could use something like:

location = /.ha-auth {
    internal;
    proxy_pass              http://supervisor/auth;
    proxy_pass_request_body off;
    proxy_set_header        Content-Length "";
    proxy_set_header        X-Supervisor-Token "${SUPERVISOR_TOKEN}"; # The ${SUPERVISOR_TOKEN} needs to be set
}

location / {
    auth_request     /.ha-auth;
    auth_request_set $auth_status $upstream_status;
    proxy_set_header  X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header  Host $http_host;
    #proxy_set_header X-Remote-User $remote_user;
    #proxy_set_header   X-Forwarded-User $http_authorization;
    proxy_pass http://127.0.0.1:5000/;  # your addon could use the X-Remote-User header with the username
}

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints of add-on configuration are added/changed:

zeehio added a commit to zeehio/developers.home-assistant that referenced this pull request Jan 7, 2021
@zeehio zeehio marked this pull request as ready for review January 7, 2021 10:31
@zeehio
Copy link
Contributor Author

zeehio commented Jan 7, 2021

Tested. Ready for review

if request.method == "GET":
raise HTTPBadRequest(
text="URL encoded auth requires HTTP POST instead of GET"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of curse, that is correct, but do we need to guide people there? We can still just accept GET without future validation if GET or POST.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed those checks. It becomes a one line pull request :-)

zeehio added a commit to zeehio/supervisor that referenced this pull request Jan 7, 2021
@pvizeli pvizeli merged commit 3b70cd5 into home-assistant:main Jan 7, 2021
@frenck
Copy link
Member

frenck commented Jan 8, 2021

Really nice addition! Thanks, @zeehio 👍

@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2021
@ludeeus ludeeus added the new-feature A new feature label Jan 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants