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

Support openid connect like authentication providers #32926

Closed
wants to merge 54 commits into from

Conversation

elupus
Copy link
Contributor

@elupus elupus commented Mar 18, 2020

Breaking change

Proposed change

Add support for using OAuth2/OpenId authentication providers like google, facebook

Type of change

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

Example entry for configuration.yaml:

# Example configuration.yaml for google openid provider
homeassistant:

  auth_providers:
    - type: openid
      name: "Google"
      client_id: "1234"
      client_secret: "4567"
      configuration: https://accounts.google.com/.well-known/openid-configuration
      emails:
        - "john.doh@gmail.com"
# Example configuration.yaml for auth0 provider supporting a multitude of underlying providers
homeassistant:

  auth_providers:
    - type: openid
      name: "Auth0"
      client_id: "1234"
      client_secret: "4567"
      configuration: https://xxx.auth0.com/.well-known/openid-configuration
      emails:
        - "john.doh@gmail.com"

Additional information

This requires changes in frontend to work correctly with external flows.

Users are whitelisted based on their (verified) email address. That however does require the email scope of openid, which we don't really need. Also email is not really considered a "stable" identifier. That said, the link between a home assistant user and the "upstream" user is based "sub" (subject) identifer. So even if email where to change, we'd still keep the link to same home assistant user.

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 homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

homeassistant/auth/providers/openid.py Outdated Show resolved Hide resolved
homeassistant/auth/providers/openid.py Outdated Show resolved Hide resolved
homeassistant/auth/providers/openid.py Outdated Show resolved Hide resolved
homeassistant/auth/providers/openid.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Mar 18, 2020
@bdraco
Copy link
Member

bdraco commented Mar 19, 2020

@elupus Looking good. Let me know when this is ready for testing

@stale
Copy link

stale bot commented Apr 29, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Apr 29, 2020
@elupus
Copy link
Contributor Author

elupus commented Apr 29, 2020

It is mainly lacking some comments on solution and on how to proceed. Not really stale.

@elupus
Copy link
Contributor Author

elupus commented May 10, 2020

@bdraco if you are interested in testing. it's about as done as i'm likely to get for now. The annoying thing is that there are fewer openid providers than i thought when i started this endever.

@elupus
Copy link
Contributor Author

elupus commented May 10, 2020

Auth0 was quite nice to use as a wrapper. The free plan allow two active authentication providers.

@bdraco
Copy link
Member

bdraco commented May 10, 2020

@elupus Looks like this needs a rebase for the new external_url / internal_url changes

2020-05-10 15:45:03 WARNING (MainThread) [homeassistant.components.http] Detected use of deprecated `base_url` property, use `homeassistant.helpers.network.get_url` method instead. Please report issue for http using this method at homeassistant/components/http/__init__.py, line 136: for frame in reversed(extract_stack()):
2020-05-10 15:45:09 WARNING (MainThread) [homeassistant.components.http] Detected use of deprecated `base_url` property, use `homeassistant.helpers.network.get_url` method instead. Please report issue for http using this method at homeassistant/components/http/__init__.py, line 136: for frame in reversed(extract_stack()):
2020-05-10 15:45:12 WARNING (MainThread) [homeassistant.components.http] Detected use of deprecated `base_url` property, use `homeassistant.helpers.network.get_url` method instead. Please report issue for http using this method at homeassistant/components/http/__init__.py, line 136: for frame in reversed(extract_stack()):

Happy to test afterwards.

@elupus
Copy link
Contributor Author

elupus commented May 10, 2020

Yea, but should still work I think, but I'll rebase it.

@bdraco
Copy link
Member

bdraco commented May 10, 2020

I gave this a shot using https://developers.google.com/identity/protocols/oauth2/openid-connect

  auth_providers:
    - type: openid
      name: "Google"
      client_id: "<SNIP>"
      client_secret: "<SNIP>"
      configuration: https://accounts.google.com/.well-known/openid-configuration
      emails:
         - "<SNIP>"

This is what I see and clicking the button returns to same page.
Screen Shot 2020-05-10 at 11 50 25 AM

@elupus
Copy link
Contributor Author

elupus commented May 10, 2020

@bdraco should be rebased. it prefers external url if set. but should work with internal url too if no external is set.

I forgot to mention, you also need the frontend pull. so a custom frontend. home-assistant/frontend#5258

@elupus
Copy link
Contributor Author

elupus commented May 10, 2020

Ps. I just rebased frontend repo. Might have been a mistake. Will clean my frontend build and see if it behaves better. but it's behaving somewhat strange.

@elupus
Copy link
Contributor Author

elupus commented May 10, 2020

Had to rebuild my frontend build after rebasing it, then it got back to working. So it should be up-to date and working.

@elupus elupus marked this pull request as ready for review May 10, 2020 18:54
@bdraco
Copy link
Member

bdraco commented May 10, 2020

Screen Shot 2020-05-10 at 4 05 50 PM

Maybe present a button to trigger the login if the window is blocked?

@bdraco
Copy link
Member

bdraco commented May 10, 2020

@elupus Once I got past the popup block, everything is working great 👍

Dev automation moved this from Review in progress to Reviewer approved May 17, 2020
@balloob
Copy link
Member

balloob commented Jan 13, 2021

Going to close this. We should first address #32926 (comment) in a new PR before we can consider this.

@balloob balloob closed this Jan 13, 2021
Dev automation moved this from Review in progress to Cancelled Jan 13, 2021
@elupus
Copy link
Contributor Author

elupus commented Jan 13, 2021

There is a pull addressing just that open..

@elupus
Copy link
Contributor Author

elupus commented Jan 13, 2021

#44630

@werdnum
Copy link

werdnum commented Feb 2, 2021

#44630 has been merged. What else is needed to reopen consideration of this PR? I've been really hoping for something like this: being able to use industry standards instead of delegating to a hacky command-line tool.

@reesericci
Copy link

I just want to create an openid connect SSO solution with keycloak

@michael-robbins
Copy link

@balloob what else is left to reopen this PR and consider it again?

@scrapid
Copy link

scrapid commented Aug 1, 2021

@balloob When MS Windows machine is connected to AD domain but it temporarily does not have access to the domain controller via network, than every user currently logged to the system sill has access to it. Even after locking screen and unlocking it using meantime expired credentials. Should MS disable Windows AD integration, because in some cases Windows is not able to verify current privileges? It's ridiculous. I prefer conscious choice over limits and prohibition. Some people just need LDAP authentication even if it has own characteristics/disadvantages. Every Home Assistant admin should know what is he doing by enabling optional features, it's admin's responsibility. Let's allow HA admins to log off users automatically after some time to enforce users to use their credentials again and fetch current account status from remote databases. Or maybe let's allow HA admins to invalidate all the particular user's sessions on the admin's request. Those two options are standard in software with LDAP authentication. I think properly and very well designed current HA authentication with it's security and advantages (I appreciate this!) shouldn't limit other awaited options. Especially when docker HA installations need additional ldap tools so they cause another problems with custom script authentication method.

@Ceralor
Copy link

Ceralor commented Aug 2, 2021

@scrapid Your comparison to a Windows machine is a far cry from affecting a live application that impacts multiple things. Accurately following your comparison would still include rejecting access when it can't confirm, as authenticating with network applications is to do. A local computer and a system impacting multiple people is not a valid comparison.

That being said, I want to throw my support in here. There are a number of other applications such as Nextcloud, etc that do have options easily implemented for LDAP, SAML, or OpenID authentication. It seems as if the current hold up is more to do with how HA currently implements authentication than to do with whether or not it's actually feasible. That is an understandable hold up, and it's totally reasonable to say "HA's underlying architecture does not make safely implementing this as easy as many seem to think, but we may open up a branch for accepting contributions on it."

@colemickens
Copy link

So then, as someone asked exactly 6 months ago, now that the blocking issue was resolved, what remains in the way of merging this?

@frenck
Copy link
Member

frenck commented Aug 2, 2021

This is more a question for our forums. This PR is closed and handled for that matter.

Thanks for understanding.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet