WIP: Add multi-factor authentication plug-able modules by awarecan · Pull Request #15335 · home-assistant/core

awarecan · 2018-07-06T23:57:40Z

Description:

Add multi-factor authentication support for new auth system.

Auth module

Multi-factor auth modules has been added to auth/modules folder.

totp is Time-based One Time Password module which compatible with Google Authenticator and Authy (not test yet).

insecure_example is a Example designed for demo and unit testing purpose, should not be used in any production system.

Multi-facot auth module can be used mixed-match with auth providers. After normal auth provider validate, if there are auth modules enabled for the specific user, the login flow will direct to auth module input form. Auth module use separated storage, can be hardening it in future.

Related issue (if applicable): fixes #

Pull request in developers.home-assistant.io with documentation (if applicable): home-assistant/developers.home-assistant#52

Example entry for `configuration.yaml` (if applicable):

# configuration.yaml
homeassistant:
  # Auth Provider
  auth_providers:
    - type: homeassistant
  auth_mfa_modules:
    - type: totp

auth:

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

Documentation added/updated in developers.home-assistant.io

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

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

awarecan · 2018-07-07T00:02:52Z

For other auth provider, such as legacy_api_password, you need to manually edit ~/.homeassistant/.storage/auth_module.totp file (the username for legacy_api_password is homeassistant)

To manually generate ota_secret, you can execute following python script

import pyotp
print(pyotp.random_base32())

balloob · 2018-07-07T16:39:08Z

homeassistant/auth.py

        """Return the name of the auth provider."""
        return self.config.get(CONF_NAME, self.DEFAULT_TITLE)

+    async def load_modules(self, moudle_configs):


balloob · 2018-07-07T16:40:47Z

homeassistant/auth_providers/homeassistant.py

+                'users': [],
            }

+        if 'users' not in data:


When can this happen?

Data file was corrupted? Cannot recall why I added this statement, can be removed if you feel redundant.

It's redundant. If storage doesn't exist, we get None back and set the default above. If we save, the default was the foundation.

balloob · 2018-07-07T21:13:19Z

It's usually wise to discuss these things before creating big PRs like this. That way there is no work wasted if we have different views on the architecture.

balloob · 2018-07-07T21:14:36Z

homeassistant/auth_providers/homeassistant.py

+            await self.hass.async_add_executor_job(
+                data.validate_login, username, password)
+        finally:
+            await data.async_save()


Why do we need to always save?

legacy code, should be removed. It was used in v1 implementation when auth module save data in auth provider storage.

awarecan · 2018-07-07T21:17:45Z

I thought we had the discussion around plugable auth module in #15225

balloob · 2018-07-09T10:07:15Z

That's true, but that's only 1 part of this. Starting code review now

balloob · 2018-07-09T10:24:38Z

homeassistant/auth.py

+        """Initialize the login flow."""
+        self._auth_provider = auth_provider
+        # self._auth_modules is mutable, we need a copy
+        self._auth_modules = auth_provider.modules.copy()


auth modules should not change anymore after startup ?

balloob · 2018-07-09T10:25:57Z

homeassistant/auth.py

+    """Raised when we encounter invalid authentication."""
+
+
+class SessionStore:


I don't see any need for the session store. This should just be stored on the instance of the login flow handler. It will then be removed with that.

Yes, it was inherited from previous version of implement. I agreed that is over-design, we should not have session at all, flow_id can be used as a session.

balloob · 2018-07-09T10:28:40Z

homeassistant/auth_providers/modules/insecure_example.py

@@ -0,0 +1,66 @@
+"""Example auth module."""


This should not live under auth provider as it has nothing to do with it?

I suggest we restructure our auth into

homeassistant/auth/__init__.py

homeassistant/auth/providers/homeassistant.py

homeassistant/auth/modules/totp.py

I also not sure if I like modules, it's too generic. What about two_factor ?

balloob · 2018-07-09T10:29:32Z

homeassistant/auth_providers/modules/insecure_example.py

+}, extra=vol.PREVENT_EXTRA)
+
+STORAGE_VERSION = 1
+STORAGE_KEY = 'auth_module.insecure_example'


I think that we should store these on the user objects in a data object that is stored/restored when user is saved/restored.

balloob · 2018-07-09T10:33:35Z

homeassistant/auth.py

        return {}


+class LoginFlow(data_entry_flow.FlowHandler):


I don't like this architecture.

I think that we should implement it as a FlowHandler that is not extended by the login flows, but instead wraps one.

It should take the login flow it needs to wrap. Once that flow returns a create entry, it should hold that value, look up the user, see if any two factor has been registered, instantiate those and let them handle the next step.

You can use __getattr__ to intercept the methods on the LoginFlow and decide where to redirect the call.

Hmm , maybe I do like this architecture. I just don't like how it iterates through the modules and mutates them.

balloob · 2018-07-09T10:35:08Z

homeassistant/auth.py

+                    if not errors and result:
+                        return await self.async_finish(result)
+                else:
+                    session_id = await auth_module.\


We shouldn't create sessions, just instantiate and store on this instance.

balloob · 2018-07-09T10:36:06Z

homeassistant/auth.py

+
+    async def async_finish(self, username):
+        """Handle the pass of login flow."""
+        if self._auth_modules:


Use a guard clause

if not X: return Y …

balloob · 2018-07-09T10:39:32Z

homeassistant/auth.py

+    async def async_finish(self, username):
+        """Handle the pass of login flow."""
+        if self._auth_modules:
+            _, auth_module = self._auth_modules.popitem(False)


This is weird, instead, once finish called, we should check if user has any 2fa configured (stored on user object?) and then create a local list of 2fa to ask which we can mutate.

A user can have multiple 2FA active, it only needs to pass 1 to log in.

I see your point, so the flow should like

admin create user -> user login -> user enable/setup one of many 2fa from a list
user select auth_provider -> auth_provider validate -> user select which 2fa he/she want to use -> ask 2fa challenge -> 2fa module validate -> user logged in

Yep, and we can do it all within a login flow without needed a frontend change

balloob · 2018-07-09T10:39:46Z

homeassistant/auth.py

+        if self._auth_modules:
+            _, auth_module = self._auth_modules.popitem(False)
+
+            step_id = 'auth_module_' + auth_module.type


string concatenation should be done using .format

balloob · 2018-07-09T10:40:26Z

homeassistant/auth.py

+        await self.async_save()
+        return data
+
+    async def async_cleanup_session(self):


Session expiration is up to the LoginFlow to enforce.

balloob · 2018-07-09T11:22:03Z

homeassistant/auth.py

    vol.Optional(CONF_NAME): str,
    # Specify ID if you have two auth providers for same type.
    vol.Optional(CONF_ID): str,
+    vol.Optional(CONF_MODULES):


Modules should not be nested under an auth provider. Instead, users should be able to enable 2fa for their account.

So the configuration should like

# configuration.yml homeassistant: auth_providers: - type: a_provder - type: b_provider auth_2fa_modules: - type: totp - type: hotp_sms

user should be able to enable one of many module in his setting page
user should be able to select use which module after auth_provider validate

That is the sequence used in my Cisco VPN client configured with okta integration.

homeassistant · 2018-07-10T00:10:37Z

Hello @awarecan,

When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).

The commits that are missing a linked GitHub account are the following:

6f9a98b674fcefd9e2d5165915da4db18529ee30 - This commit has something that looks like an email address (ec2-user@ip-172-31-28-229.us-west-2.compute.internal). Maybe try linking that to GitHub?.

Unfortunately, we are unable to accept this pull request until this situation is corrected.

Here are your options:

If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
```
git commit --amend --author="Author Name <email@address.com>"
```
  (substituting Author Name and email@address.com for your actual information) to set the authorship information.
- If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
  1. You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
  2. You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.

We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.

Thanks, I look forward to checking this PR again soon! ❤️

…ded errors (home-assistant#15414)

* support for tuya platform * support tuya platform * lint fix * change dependency * add tuya platform support * remove tuya platform except switch. fix code as required * fix the code as review required * fix as required * fix a mistake

* Update __init__.py * Update requirements_all.txt

There are some devices that speak HomeKit that we shouldn't expose. Some bridges (such as the Hue) provide reduced functionality over HomeKit and have a functional native API, so should be ignored. We also shouldn't offer to configure the built-in Home Assistant HomeKit bridge.

…me-assistant#15298)

* Add HomematicIP security zone * Update access point tests * Fix state if not armed and coments * Add comment for the empty state_attributes * Fix comment * Fix spelling

…istant#15438)

* Add Python 3.8-dev tox tests. * Allow failures on 3.8-dev * Allow failures on 3.8-dev take2 * Only run on pushes to dev

## Description: Make typing checks more strict: add `--strict-optional` flag that forbids implicit None return type. This flag will become default in the next version of mypy (0.600) Add `homeassistant/util/` to checked dirs. ## Checklist: - [x] The code change is tested and works locally. - [x] Local tests pass with `tox`. **Your PR cannot be merged unless tests pass**

* Upgrade mypy to 0.600 * Upgrade mypy to 0.610 * Typing improvements * remove unneeded or * remove merge artifact * Update loader.py

* User management * Lint * Fix dict * Reuse data instance * OrderedDict all the way

Only include base class and insecure_example

awarecan · 2018-07-16T13:25:14Z

Rebase seems mess up, close this PR for now

awarecan requested a review from a team as a code owner July 6, 2018 23:57

homeassistant added cla-signed core labels Jul 6, 2018

ghost added the in progress label Jul 6, 2018

awarecan mentioned this pull request Jul 7, 2018

Enable two factor authentication #15225

Closed

7 tasks

balloob reviewed Jul 7, 2018

View reviewed changes

homeassistant added the cla-signed label Jul 7, 2018

awarecan mentioned this pull request Jul 7, 2018

Update auth document home-assistant/developers.home-assistant#52

Merged

balloob reviewed Jul 7, 2018

View reviewed changes

balloob reviewed Jul 9, 2018

View reviewed changes

homeassistant added the cla-error label Jul 10, 2018

awarecan force-pushed the auth-2fa-v2 branch from 6f9a98b to be2114a Compare July 10, 2018 00:15

homeassistant added the cla-signed label Jul 10, 2018

awarecan force-pushed the auth-2fa-v2 branch from be2114a to 9dbd88d Compare July 10, 2018 00:22

tchellomello and others added 16 commits July 16, 2018 05:28

Change Ring binary_sensor frequency polling to avoid rate limit excee…

62a210f

…ded errors (home-assistant#15414)

Fix HomeMatic variables (home-assistant#15417)

8726856

* Update __init__.py * Update requirements_all.txt

Make LimitlessLED color/temperature attributes mutually exclusive (ho…

4e73d49

…me-assistant#15298)

Add HomematicIP alarm control panel (home-assistant#15342)

d671657

* Add HomematicIP security zone * Update access point tests * Fix state if not armed and coments * Add comment for the empty state_attributes * Fix comment * Fix spelling

Include request.path in legacy api password warning message (home-ass…

4464ce8

…istant#15438)

Add python 3.8-dev to travis and tox (home-assistant#15347)

75adf0f

* Add Python 3.8-dev tox tests. * Allow failures on 3.8-dev * Allow failures on 3.8-dev take2 * Only run on pushes to dev

Reorg auth (home-assistant#15443)

ce4f049

upgrade-mypy (home-assistant#14904)

adb9d68

* Upgrade mypy to 0.600 * Upgrade mypy to 0.610 * Typing improvements * remove unneeded or * remove merge artifact * Update loader.py

Fix comment formatting (home-assistant#15447)

8fc9b5b

User management (home-assistant#15420)

d4ac9fb

* User management * Lint * Fix dict * Reuse data instance * OrderedDict all the way

Bump frontend to 20180713.0

5b17663

Add multi factor authentication support

56ab019

Only include base class and insecure_example

Resolve code review comment

96678bd

awarecan force-pushed the auth-2fa-v2 branch from 27372e4 to 96678bd Compare July 16, 2018 13:23

awarecan requested review from Danielhiversen, Kane610, bachya, dgomes, rytilahti and syssi as code owners July 16, 2018 13:23

awarecan requested a review from a team July 16, 2018 13:23

homeassistant added the cla-signed label Jul 16, 2018

awarecan changed the title ~~Add multi-factor authentication plug-able modules~~ WIP: Add multi-factor authentication plug-able modules Jul 16, 2018

awarecan closed this Jul 16, 2018

ghost removed the in progress label Jul 16, 2018

home-assistant locked and limited conversation to collaborators Dec 10, 2018

		"""Raised when we encounter invalid authentication."""


		class SessionStore:

Uh oh!

Conversation

awarecan commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Auth module

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

awarecan commented Jul 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Jul 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awarecan commented Jul 7, 2018

Uh oh!

balloob commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

homeassistant commented Jul 10, 2018

Uh oh!

awarecan commented Jul 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

awarecan commented Jul 6, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

balloob Jul 9, 2018 •

edited

Loading